diff mbox series

[v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065]

Message ID 20230131213655.4033602-1-goldstein.w.n@gmail.com
State Superseded
Headers
Series [v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Noah Goldstein Jan. 31, 2023, 9:36 p.m. UTC 
  Two issue:

1) Zero-length check is doing:
```
    test    %rdx, %rdx
    jl      L(zero_len)
```
which doesn't actually check zero (was at some point `decq` and the
flag never got updated).

The fix is just make the flag `jle` i.e:
```
    test    %rdx, %rdx
    jle     L(zero_len)
```

2) Length check in page-cross case checking if we should continue is
doing:
```
    cmpq    %r8, %rdx
    jb      L(page_cross_small)
```
which means we will continue searching for null-term if length ends at
the end of a page and there was no null-term in `src`.

The fix is to make the flag:
```
    cmpq    %r8, %rdx
    jbe     L(page_cross_small)
```
---
 string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
 sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

H.J. Lu Jan. 31, 2023, 10:28 p.m. UTC | #1 
On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> Two issue:
>
> 1) Zero-length check is doing:
> ```
>     test    %rdx, %rdx
>     jl      L(zero_len)
> ```
> which doesn't actually check zero (was at some point `decq` and the
> flag never got updated).
>
> The fix is just make the flag `jle` i.e:
> ```
>     test    %rdx, %rdx
>     jle     L(zero_len)
> ```
>
> 2) Length check in page-cross case checking if we should continue is
> doing:
> ```
>     cmpq    %r8, %rdx
>     jb      L(page_cross_small)
> ```
> which means we will continue searching for null-term if length ends at
> the end of a page and there was no null-term in `src`.

It is not purely about null-term.   strncat shouldn't read beyond the limit.
In this case, src may point to PROT_NONE memory.

> The fix is to make the flag:
> ```
>     cmpq    %r8, %rdx
>     jbe     L(page_cross_small)
> ```
> ---
>  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
>  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/string/test-strncat.c b/string/test-strncat.c
> index e03d329e1c..c0cde206ee 100644
> --- a/string/test-strncat.c
> +++ b/string/test-strncat.c
> @@ -28,6 +28,7 @@
>  # define CHAR char
>  # define UCHAR unsigned char
>  # define SIMPLE_STRNCAT simple_strncat
> +# define STRNLEN strnlen
>  # define STRLEN strlen
>  # define MEMSET memset
>  # define MEMCPY memcpy
> @@ -40,6 +41,7 @@
>  # define CHAR wchar_t
>  # define UCHAR wchar_t
>  # define SIMPLE_STRNCAT simple_wcsncat
> +# define STRNLEN wcsnlen
>  # define STRLEN wcslen
>  # define MEMSET wmemset
>  # define MEMCPY wmemcpy
> @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
>        return;
>      }
>
> -  size_t len = STRLEN (src);
> +  size_t len = STRNLEN (src, n);
>    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)

>      {
>        error (0, 0, "Incorrect concatenation in function %s",
> @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
>      }
>  }
>
> +static void
> +do_test_src_no_nullterm_bz30065 (void)
> +{
> +  /* NB: "src does not need to be null-terminated if it contains n or more
> +   * bytes." */
> +  CHAR *s1, *s2;
> +  size_t bound = page_size / sizeof (CHAR);
> +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> +  s2 = (CHAR *) buf2;
> +  MEMSET (s1 - bound, -1, bound);
> +  for (size_t n = 0; n < bound; ++n)
> +    {
> +      FOR_EACH_IMPL (impl, 0)
> +       {
> +         s2[0] = '\0';
> +         do_one_test (impl, s2, s1 - n, n);
> +       }
> +    }
> +}
> +
>  static void
>  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
>          size_t n, int max_char)
> @@ -372,6 +394,7 @@ test_main (void)
>
>    do_random_tests ();
>    do_overflow_tests ();
> +  do_test_src_no_nullterm_bz30065 ();
>    return ret;
>  }
>
> diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> index b380e8e11c..c2ff202238 100644
> --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
>         salq    $2, %rdx
>  # else
>         test    %rdx, %rdx
> -       jl      L(zero_len)
> +       jle     L(zero_len)
>  # endif
>         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
>
> @@ -387,7 +387,7 @@ L(page_cross):
>         subl    %esi, %r8d
>         andl    $(VEC_SIZE - 1), %r8d
>         cmpq    %r8, %rdx
> -       jb      L(page_cross_small)
> +       jbe     L(page_cross_small)
>
>         /* Optimizing more aggressively for space as this is very cold
>            code. This saves 2x cache lines.  */
> --
> 2.34.1
>
Noah Goldstein Jan. 31, 2023, 10:37 p.m. UTC | #2 
On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > Two issue:
> >
> > 1) Zero-length check is doing:
> > ```
> >     test    %rdx, %rdx
> >     jl      L(zero_len)
> > ```
> > which doesn't actually check zero (was at some point `decq` and the
> > flag never got updated).
> >
> > The fix is just make the flag `jle` i.e:
> > ```
> >     test    %rdx, %rdx
> >     jle     L(zero_len)
> > ```
> >
> > 2) Length check in page-cross case checking if we should continue is
> > doing:
> > ```
> >     cmpq    %r8, %rdx
> >     jb      L(page_cross_small)
> > ```
> > which means we will continue searching for null-term if length ends at
> > the end of a page and there was no null-term in `src`.
>
> It is not purely about null-term.   strncat shouldn't read beyond the limit.
> In this case, src may point to PROT_NONE memory.
>

If there was a null-term the later check:
```
shll $CHAR_SIZE, %ecx
jz L(page_cross_continue)
```
would catch it, this can only occur if there is no null-term.
> > The fix is to make the flag:
> > ```
> >     cmpq    %r8, %rdx
> >     jbe     L(page_cross_small)
> > ```
> > ---
> >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > index e03d329e1c..c0cde206ee 100644
> > --- a/string/test-strncat.c
> > +++ b/string/test-strncat.c
> > @@ -28,6 +28,7 @@
> >  # define CHAR char
> >  # define UCHAR unsigned char
> >  # define SIMPLE_STRNCAT simple_strncat
> > +# define STRNLEN strnlen
> >  # define STRLEN strlen
> >  # define MEMSET memset
> >  # define MEMCPY memcpy
> > @@ -40,6 +41,7 @@
> >  # define CHAR wchar_t
> >  # define UCHAR wchar_t
> >  # define SIMPLE_STRNCAT simple_wcsncat
> > +# define STRNLEN wcsnlen
> >  # define STRLEN wcslen
> >  # define MEMSET wmemset
> >  # define MEMCPY wmemcpy
> > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> >        return;
> >      }
> >
> > -  size_t len = STRLEN (src);
> > +  size_t len = STRNLEN (src, n);
> >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
>
> >      {
> >        error (0, 0, "Incorrect concatenation in function %s",
> > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> >      }
> >  }
> >
> > +static void
> > +do_test_src_no_nullterm_bz30065 (void)
> > +{
> > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > +   * bytes." */
> > +  CHAR *s1, *s2;
> > +  size_t bound = page_size / sizeof (CHAR);
> > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > +  s2 = (CHAR *) buf2;
> > +  MEMSET (s1 - bound, -1, bound);
> > +  for (size_t n = 0; n < bound; ++n)
> > +    {
> > +      FOR_EACH_IMPL (impl, 0)
> > +       {
> > +         s2[0] = '\0';
> > +         do_one_test (impl, s2, s1 - n, n);
> > +       }
> > +    }
> > +}
> > +
> >  static void
> >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> >          size_t n, int max_char)
> > @@ -372,6 +394,7 @@ test_main (void)
> >
> >    do_random_tests ();
> >    do_overflow_tests ();
> > +  do_test_src_no_nullterm_bz30065 ();
> >    return ret;
> >  }
> >
> > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > index b380e8e11c..c2ff202238 100644
> > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> >         salq    $2, %rdx
> >  # else
> >         test    %rdx, %rdx
> > -       jl      L(zero_len)
> > +       jle     L(zero_len)
> >  # endif
> >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> >
> > @@ -387,7 +387,7 @@ L(page_cross):
> >         subl    %esi, %r8d
> >         andl    $(VEC_SIZE - 1), %r8d
> >         cmpq    %r8, %rdx
> > -       jb      L(page_cross_small)
> > +       jbe     L(page_cross_small)
> >
> >         /* Optimizing more aggressively for space as this is very cold
> >            code. This saves 2x cache lines.  */
> > --
> > 2.34.1
> >
>
>
> --
> H.J.
H.J. Lu Jan. 31, 2023, 10:42 p.m. UTC | #3 
On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > >
> > > Two issue:
> > >
> > > 1) Zero-length check is doing:
> > > ```
> > >     test    %rdx, %rdx
> > >     jl      L(zero_len)
> > > ```
> > > which doesn't actually check zero (was at some point `decq` and the
> > > flag never got updated).
> > >
> > > The fix is just make the flag `jle` i.e:
> > > ```
> > >     test    %rdx, %rdx
> > >     jle     L(zero_len)
> > > ```
> > >
> > > 2) Length check in page-cross case checking if we should continue is
> > > doing:
> > > ```
> > >     cmpq    %r8, %rdx
> > >     jb      L(page_cross_small)
> > > ```
> > > which means we will continue searching for null-term if length ends at
> > > the end of a page and there was no null-term in `src`.
> >
> > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > In this case, src may point to PROT_NONE memory.
> >
>
> If there was a null-term the later check:
> ```
> shll $CHAR_SIZE, %ecx
> jz L(page_cross_continue)
> ```
> would catch it, this can only occur if there is no null-term.

But it is incorrect to read beyond the limit even if there is a null-term.

> > > The fix is to make the flag:
> > > ```
> > >     cmpq    %r8, %rdx
> > >     jbe     L(page_cross_small)
> > > ```
> > > ---
> > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > index e03d329e1c..c0cde206ee 100644
> > > --- a/string/test-strncat.c
> > > +++ b/string/test-strncat.c
> > > @@ -28,6 +28,7 @@
> > >  # define CHAR char
> > >  # define UCHAR unsigned char
> > >  # define SIMPLE_STRNCAT simple_strncat
> > > +# define STRNLEN strnlen
> > >  # define STRLEN strlen
> > >  # define MEMSET memset
> > >  # define MEMCPY memcpy
> > > @@ -40,6 +41,7 @@
> > >  # define CHAR wchar_t
> > >  # define UCHAR wchar_t
> > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > +# define STRNLEN wcsnlen
> > >  # define STRLEN wcslen
> > >  # define MEMSET wmemset
> > >  # define MEMCPY wmemcpy
> > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > >        return;
> > >      }
> > >
> > > -  size_t len = STRLEN (src);
> > > +  size_t len = STRNLEN (src, n);
> > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> >
> > >      {
> > >        error (0, 0, "Incorrect concatenation in function %s",
> > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > >      }
> > >  }
> > >
> > > +static void
> > > +do_test_src_no_nullterm_bz30065 (void)
> > > +{
> > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > +   * bytes." */
> > > +  CHAR *s1, *s2;
> > > +  size_t bound = page_size / sizeof (CHAR);
> > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > +  s2 = (CHAR *) buf2;
> > > +  MEMSET (s1 - bound, -1, bound);
> > > +  for (size_t n = 0; n < bound; ++n)
> > > +    {
> > > +      FOR_EACH_IMPL (impl, 0)
> > > +       {
> > > +         s2[0] = '\0';
> > > +         do_one_test (impl, s2, s1 - n, n);
> > > +       }
> > > +    }
> > > +}
> > > +
> > >  static void
> > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > >          size_t n, int max_char)
> > > @@ -372,6 +394,7 @@ test_main (void)
> > >
> > >    do_random_tests ();
> > >    do_overflow_tests ();
> > > +  do_test_src_no_nullterm_bz30065 ();
> > >    return ret;
> > >  }
> > >
> > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > index b380e8e11c..c2ff202238 100644
> > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > >         salq    $2, %rdx
> > >  # else
> > >         test    %rdx, %rdx
> > > -       jl      L(zero_len)
> > > +       jle     L(zero_len)
> > >  # endif
> > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > >
> > > @@ -387,7 +387,7 @@ L(page_cross):
> > >         subl    %esi, %r8d
> > >         andl    $(VEC_SIZE - 1), %r8d
> > >         cmpq    %r8, %rdx
> > > -       jb      L(page_cross_small)
> > > +       jbe     L(page_cross_small)
> > >
> > >         /* Optimizing more aggressively for space as this is very cold
> > >            code. This saves 2x cache lines.  */
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > H.J.
Noah Goldstein Jan. 31, 2023, 10:52 p.m. UTC | #4 
On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > >
> > > > Two issue:
> > > >
> > > > 1) Zero-length check is doing:
> > > > ```
> > > >     test    %rdx, %rdx
> > > >     jl      L(zero_len)
> > > > ```
> > > > which doesn't actually check zero (was at some point `decq` and the
> > > > flag never got updated).
> > > >
> > > > The fix is just make the flag `jle` i.e:
> > > > ```
> > > >     test    %rdx, %rdx
> > > >     jle     L(zero_len)
> > > > ```
> > > >
> > > > 2) Length check in page-cross case checking if we should continue is
> > > > doing:
> > > > ```
> > > >     cmpq    %r8, %rdx
> > > >     jb      L(page_cross_small)
> > > > ```
> > > > which means we will continue searching for null-term if length ends at
> > > > the end of a page and there was no null-term in `src`.
> > >
> > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > In this case, src may point to PROT_NONE memory.
> > >
> >
> > If there was a null-term the later check:
> > ```
> > shll $CHAR_SIZE, %ecx
> > jz L(page_cross_continue)
> > ```
> > would catch it, this can only occur if there is no null-term.
>
> But it is incorrect to read beyond the limit even if there is a null-term.
It's a page-aligned read where len != 0 so the original read should be fine.

The `jb L(page_cross_small)`
falls through to the null-term check, so if there was a null-term we won't
read anymore bytes.
>
> > > > The fix is to make the flag:
> > > > ```
> > > >     cmpq    %r8, %rdx
> > > >     jbe     L(page_cross_small)
> > > > ```
> > > > ---
> > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > index e03d329e1c..c0cde206ee 100644
> > > > --- a/string/test-strncat.c
> > > > +++ b/string/test-strncat.c
> > > > @@ -28,6 +28,7 @@
> > > >  # define CHAR char
> > > >  # define UCHAR unsigned char
> > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > +# define STRNLEN strnlen
> > > >  # define STRLEN strlen
> > > >  # define MEMSET memset
> > > >  # define MEMCPY memcpy
> > > > @@ -40,6 +41,7 @@
> > > >  # define CHAR wchar_t
> > > >  # define UCHAR wchar_t
> > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > +# define STRNLEN wcsnlen
> > > >  # define STRLEN wcslen
> > > >  # define MEMSET wmemset
> > > >  # define MEMCPY wmemcpy
> > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > >        return;
> > > >      }
> > > >
> > > > -  size_t len = STRLEN (src);
> > > > +  size_t len = STRNLEN (src, n);
> > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > >
> > > >      {
> > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > >      }
> > > >  }
> > > >
> > > > +static void
> > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > +{
> > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > +   * bytes." */
> > > > +  CHAR *s1, *s2;
> > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > +  s2 = (CHAR *) buf2;
> > > > +  MEMSET (s1 - bound, -1, bound);
> > > > +  for (size_t n = 0; n < bound; ++n)
> > > > +    {
> > > > +      FOR_EACH_IMPL (impl, 0)
> > > > +       {
> > > > +         s2[0] = '\0';
> > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > +       }
> > > > +    }
> > > > +}
> > > > +
> > > >  static void
> > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > >          size_t n, int max_char)
> > > > @@ -372,6 +394,7 @@ test_main (void)
> > > >
> > > >    do_random_tests ();
> > > >    do_overflow_tests ();
> > > > +  do_test_src_no_nullterm_bz30065 ();
> > > >    return ret;
> > > >  }
> > > >
> > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > index b380e8e11c..c2ff202238 100644
> > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > >         salq    $2, %rdx
> > > >  # else
> > > >         test    %rdx, %rdx
> > > > -       jl      L(zero_len)
> > > > +       jle     L(zero_len)
> > > >  # endif
> > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > >
> > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > >         subl    %esi, %r8d
> > > >         andl    $(VEC_SIZE - 1), %r8d
> > > >         cmpq    %r8, %rdx
> > > > -       jb      L(page_cross_small)
> > > > +       jbe     L(page_cross_small)
> > > >
> > > >         /* Optimizing more aggressively for space as this is very cold
> > > >            code. This saves 2x cache lines.  */
> > > > --
> > > > 2.34.1
> > > >
> > >
> > >
> > > --
> > > H.J.
>
>
>
> --
> H.J.
H.J. Lu Jan. 31, 2023, 11:01 p.m. UTC | #5 
On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > >
> > > > > Two issue:
> > > > >
> > > > > 1) Zero-length check is doing:
> > > > > ```
> > > > >     test    %rdx, %rdx
> > > > >     jl      L(zero_len)
> > > > > ```
> > > > > which doesn't actually check zero (was at some point `decq` and the
> > > > > flag never got updated).
> > > > >
> > > > > The fix is just make the flag `jle` i.e:
> > > > > ```
> > > > >     test    %rdx, %rdx
> > > > >     jle     L(zero_len)
> > > > > ```
> > > > >
> > > > > 2) Length check in page-cross case checking if we should continue is
> > > > > doing:
> > > > > ```
> > > > >     cmpq    %r8, %rdx
> > > > >     jb      L(page_cross_small)
> > > > > ```
> > > > > which means we will continue searching for null-term if length ends at
> > > > > the end of a page and there was no null-term in `src`.
> > > >
> > > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > > In this case, src may point to PROT_NONE memory.
> > > >
> > >
> > > If there was a null-term the later check:
> > > ```
> > > shll $CHAR_SIZE, %ecx
> > > jz L(page_cross_continue)
> > > ```
> > > would catch it, this can only occur if there is no null-term.
> >
> > But it is incorrect to read beyond the limit even if there is a null-term.
> It's a page-aligned read where len != 0 so the original read should be fine.
>
> The `jb L(page_cross_small)`
> falls through to the null-term check, so if there was a null-term we won't
> read anymore bytes.

There is no need to search null-term for zero length case.

> >
> > > > > The fix is to make the flag:
> > > > > ```
> > > > >     cmpq    %r8, %rdx
> > > > >     jbe     L(page_cross_small)
> > > > > ```
> > > > > ---
> > > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > > index e03d329e1c..c0cde206ee 100644
> > > > > --- a/string/test-strncat.c
> > > > > +++ b/string/test-strncat.c
> > > > > @@ -28,6 +28,7 @@
> > > > >  # define CHAR char
> > > > >  # define UCHAR unsigned char
> > > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > > +# define STRNLEN strnlen
> > > > >  # define STRLEN strlen
> > > > >  # define MEMSET memset
> > > > >  # define MEMCPY memcpy
> > > > > @@ -40,6 +41,7 @@
> > > > >  # define CHAR wchar_t
> > > > >  # define UCHAR wchar_t
> > > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > > +# define STRNLEN wcsnlen
> > > > >  # define STRLEN wcslen
> > > > >  # define MEMSET wmemset
> > > > >  # define MEMCPY wmemcpy
> > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > >        return;
> > > > >      }
> > > > >
> > > > > -  size_t len = STRLEN (src);
> > > > > +  size_t len = STRNLEN (src, n);
> > > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > > >
> > > > >      {
> > > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > >      }
> > > > >  }
> > > > >
> > > > > +static void
> > > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > > +{
> > > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > > +   * bytes." */
> > > > > +  CHAR *s1, *s2;
> > > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > > +  s2 = (CHAR *) buf2;
> > > > > +  MEMSET (s1 - bound, -1, bound);
> > > > > +  for (size_t n = 0; n < bound; ++n)
> > > > > +    {
> > > > > +      FOR_EACH_IMPL (impl, 0)
> > > > > +       {
> > > > > +         s2[0] = '\0';
> > > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > > +       }
> > > > > +    }
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > > >          size_t n, int max_char)
> > > > > @@ -372,6 +394,7 @@ test_main (void)
> > > > >
> > > > >    do_random_tests ();
> > > > >    do_overflow_tests ();
> > > > > +  do_test_src_no_nullterm_bz30065 ();
> > > > >    return ret;
> > > > >  }
> > > > >
> > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > index b380e8e11c..c2ff202238 100644
> > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > > >         salq    $2, %rdx
> > > > >  # else
> > > > >         test    %rdx, %rdx
> > > > > -       jl      L(zero_len)
> > > > > +       jle     L(zero_len)
> > > > >  # endif
> > > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > > >
> > > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > > >         subl    %esi, %r8d
> > > > >         andl    $(VEC_SIZE - 1), %r8d
> > > > >         cmpq    %r8, %rdx
> > > > > -       jb      L(page_cross_small)
> > > > > +       jbe     L(page_cross_small)
> > > > >
> > > > >         /* Optimizing more aggressively for space as this is very cold
> > > > >            code. This saves 2x cache lines.  */
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > > >
> > > > --
> > > > H.J.
> >
> >
> >
> > --
> > H.J.
Noah Goldstein Jan. 31, 2023, 11:09 p.m. UTC | #6 
On Tue, Jan 31, 2023 at 5:02 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > > >
> > > > > > Two issue:
> > > > > >
> > > > > > 1) Zero-length check is doing:
> > > > > > ```
> > > > > >     test    %rdx, %rdx
> > > > > >     jl      L(zero_len)
> > > > > > ```
> > > > > > which doesn't actually check zero (was at some point `decq` and the
> > > > > > flag never got updated).
> > > > > >
> > > > > > The fix is just make the flag `jle` i.e:
> > > > > > ```
> > > > > >     test    %rdx, %rdx
> > > > > >     jle     L(zero_len)
> > > > > > ```
> > > > > >
> > > > > > 2) Length check in page-cross case checking if we should continue is
> > > > > > doing:
> > > > > > ```
> > > > > >     cmpq    %r8, %rdx
> > > > > >     jb      L(page_cross_small)
> > > > > > ```
> > > > > > which means we will continue searching for null-term if length ends at
> > > > > > the end of a page and there was no null-term in `src`.
> > > > >
> > > > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > > > In this case, src may point to PROT_NONE memory.
> > > > >
> > > >
> > > > If there was a null-term the later check:
> > > > ```
> > > > shll $CHAR_SIZE, %ecx
> > > > jz L(page_cross_continue)
> > > > ```
> > > > would catch it, this can only occur if there is no null-term.
> > >
> > > But it is incorrect to read beyond the limit even if there is a null-term.
> > It's a page-aligned read where len != 0 so the original read should be fine.
> >
> > The `jb L(page_cross_small)`
> > falls through to the null-term check, so if there was a null-term we won't
> > read anymore bytes.
>
> There is no need to search null-term for zero length case.
>
This isn't the zero length case, this is the page-cross case. Length
is != 0 here.

> > >
> > > > > > The fix is to make the flag:
> > > > > > ```
> > > > > >     cmpq    %r8, %rdx
> > > > > >     jbe     L(page_cross_small)
> > > > > > ```
> > > > > > ---
> > > > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > > > index e03d329e1c..c0cde206ee 100644
> > > > > > --- a/string/test-strncat.c
> > > > > > +++ b/string/test-strncat.c
> > > > > > @@ -28,6 +28,7 @@
> > > > > >  # define CHAR char
> > > > > >  # define UCHAR unsigned char
> > > > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > > > +# define STRNLEN strnlen
> > > > > >  # define STRLEN strlen
> > > > > >  # define MEMSET memset
> > > > > >  # define MEMCPY memcpy
> > > > > > @@ -40,6 +41,7 @@
> > > > > >  # define CHAR wchar_t
> > > > > >  # define UCHAR wchar_t
> > > > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > > > +# define STRNLEN wcsnlen
> > > > > >  # define STRLEN wcslen
> > > > > >  # define MEMSET wmemset
> > > > > >  # define MEMCPY wmemcpy
> > > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > >        return;
> > > > > >      }
> > > > > >
> > > > > > -  size_t len = STRLEN (src);
> > > > > > +  size_t len = STRNLEN (src, n);
> > > > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > > > >
> > > > > >      {
> > > > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > >      }
> > > > > >  }
> > > > > >
> > > > > > +static void
> > > > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > > > +{
> > > > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > > > +   * bytes." */
> > > > > > +  CHAR *s1, *s2;
> > > > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > > > +  s2 = (CHAR *) buf2;
> > > > > > +  MEMSET (s1 - bound, -1, bound);
> > > > > > +  for (size_t n = 0; n < bound; ++n)
> > > > > > +    {
> > > > > > +      FOR_EACH_IMPL (impl, 0)
> > > > > > +       {
> > > > > > +         s2[0] = '\0';
> > > > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > > > +       }
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > >  static void
> > > > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > > > >          size_t n, int max_char)
> > > > > > @@ -372,6 +394,7 @@ test_main (void)
> > > > > >
> > > > > >    do_random_tests ();
> > > > > >    do_overflow_tests ();
> > > > > > +  do_test_src_no_nullterm_bz30065 ();
> > > > > >    return ret;
> > > > > >  }
> > > > > >
> > > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > index b380e8e11c..c2ff202238 100644
> > > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > > > >         salq    $2, %rdx
> > > > > >  # else
> > > > > >         test    %rdx, %rdx
> > > > > > -       jl      L(zero_len)
> > > > > > +       jle     L(zero_len)
> > > > > >  # endif
> > > > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > > > >
> > > > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > > > >         subl    %esi, %r8d
> > > > > >         andl    $(VEC_SIZE - 1), %r8d
> > > > > >         cmpq    %r8, %rdx
> > > > > > -       jb      L(page_cross_small)
> > > > > > +       jbe     L(page_cross_small)
> > > > > >
> > > > > >         /* Optimizing more aggressively for space as this is very cold
> > > > > >            code. This saves 2x cache lines.  */
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > H.J.
> > >
> > >
> > >
> > > --
> > > H.J.
>
>
>
> --
> H.J.
H.J. Lu Jan. 31, 2023, 11:40 p.m. UTC | #7 
On Tue, Jan 31, 2023 at 3:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 5:02 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > > > >
> > > > > > > Two issue:
> > > > > > >
> > > > > > > 1) Zero-length check is doing:
> > > > > > > ```
> > > > > > >     test    %rdx, %rdx
> > > > > > >     jl      L(zero_len)
> > > > > > > ```
> > > > > > > which doesn't actually check zero (was at some point `decq` and the
> > > > > > > flag never got updated).
> > > > > > >
> > > > > > > The fix is just make the flag `jle` i.e:
> > > > > > > ```
> > > > > > >     test    %rdx, %rdx
> > > > > > >     jle     L(zero_len)
> > > > > > > ```
> > > > > > >
> > > > > > > 2) Length check in page-cross case checking if we should continue is
> > > > > > > doing:
> > > > > > > ```
> > > > > > >     cmpq    %r8, %rdx
> > > > > > >     jb      L(page_cross_small)
> > > > > > > ```
> > > > > > > which means we will continue searching for null-term if length ends at
> > > > > > > the end of a page and there was no null-term in `src`.
> > > > > >
> > > > > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > > > > In this case, src may point to PROT_NONE memory.
> > > > > >
> > > > >
> > > > > If there was a null-term the later check:
> > > > > ```
> > > > > shll $CHAR_SIZE, %ecx
> > > > > jz L(page_cross_continue)
> > > > > ```
> > > > > would catch it, this can only occur if there is no null-term.
> > > >
> > > > But it is incorrect to read beyond the limit even if there is a null-term.
> > > It's a page-aligned read where len != 0 so the original read should be fine.
> > >
> > > The `jb L(page_cross_small)`
> > > falls through to the null-term check, so if there was a null-term we won't
> > > read anymore bytes.
> >
> > There is no need to search null-term for zero length case.
> >
> This isn't the zero length case, this is the page-cross case. Length
> is != 0 here.

In the zero length case:

        test    %rdx, %rdx
        jl      L(zero_len)

it crashes when src points to an unreadable memory and the limit is 0.
strncat shouldn't read beyond the limit.  The change is OK.   But the
commit log should mention "avoid reading beyond the limit."

> > > >
> > > > > > > The fix is to make the flag:
> > > > > > > ```
> > > > > > >     cmpq    %r8, %rdx
> > > > > > >     jbe     L(page_cross_small)
> > > > > > > ```
> > > > > > > ---
> > > > > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > > > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > > > > index e03d329e1c..c0cde206ee 100644
> > > > > > > --- a/string/test-strncat.c
> > > > > > > +++ b/string/test-strncat.c
> > > > > > > @@ -28,6 +28,7 @@
> > > > > > >  # define CHAR char
> > > > > > >  # define UCHAR unsigned char
> > > > > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > > > > +# define STRNLEN strnlen
> > > > > > >  # define STRLEN strlen
> > > > > > >  # define MEMSET memset
> > > > > > >  # define MEMCPY memcpy
> > > > > > > @@ -40,6 +41,7 @@
> > > > > > >  # define CHAR wchar_t
> > > > > > >  # define UCHAR wchar_t
> > > > > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > > > > +# define STRNLEN wcsnlen
> > > > > > >  # define STRLEN wcslen
> > > > > > >  # define MEMSET wmemset
> > > > > > >  # define MEMCPY wmemcpy
> > > > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > >        return;
> > > > > > >      }
> > > > > > >
> > > > > > > -  size_t len = STRLEN (src);
> > > > > > > +  size_t len = STRNLEN (src, n);
> > > > > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > > > > >
> > > > > > >      {
> > > > > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > >      }
> > > > > > >  }
> > > > > > >
> > > > > > > +static void
> > > > > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > > > > +{
> > > > > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > > > > +   * bytes." */
> > > > > > > +  CHAR *s1, *s2;
> > > > > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > > > > +  s2 = (CHAR *) buf2;
> > > > > > > +  MEMSET (s1 - bound, -1, bound);
> > > > > > > +  for (size_t n = 0; n < bound; ++n)
> > > > > > > +    {
> > > > > > > +      FOR_EACH_IMPL (impl, 0)
> > > > > > > +       {
> > > > > > > +         s2[0] = '\0';
> > > > > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > > > > +       }
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void
> > > > > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > > > > >          size_t n, int max_char)
> > > > > > > @@ -372,6 +394,7 @@ test_main (void)
> > > > > > >
> > > > > > >    do_random_tests ();
> > > > > > >    do_overflow_tests ();
> > > > > > > +  do_test_src_no_nullterm_bz30065 ();
> > > > > > >    return ret;
> > > > > > >  }
> > > > > > >
> > > > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > index b380e8e11c..c2ff202238 100644
> > > > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > > > > >         salq    $2, %rdx
> > > > > > >  # else
> > > > > > >         test    %rdx, %rdx
> > > > > > > -       jl      L(zero_len)
> > > > > > > +       jle     L(zero_len)
> > > > > > >  # endif
> > > > > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > > > > >
> > > > > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > > > > >         subl    %esi, %r8d
> > > > > > >         andl    $(VEC_SIZE - 1), %r8d
> > > > > > >         cmpq    %r8, %rdx
> > > > > > > -       jb      L(page_cross_small)
> > > > > > > +       jbe     L(page_cross_small)
> > > > > > >
> > > > > > >         /* Optimizing more aggressively for space as this is very cold
> > > > > > >            code. This saves 2x cache lines.  */
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > H.J.
> > > >
> > > >
> > > >
> > > > --
> > > > H.J.
> >
> >
> >
> > --
> > H.J.
Noah Goldstein Jan. 31, 2023, 11:47 p.m. UTC | #8 
On Tue, Jan 31, 2023 at 5:40 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 3:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 5:02 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Two issue:
> > > > > > > >
> > > > > > > > 1) Zero-length check is doing:
> > > > > > > > ```
> > > > > > > >     test    %rdx, %rdx
> > > > > > > >     jl      L(zero_len)
> > > > > > > > ```
> > > > > > > > which doesn't actually check zero (was at some point `decq` and the
> > > > > > > > flag never got updated).
> > > > > > > >
> > > > > > > > The fix is just make the flag `jle` i.e:
> > > > > > > > ```
> > > > > > > >     test    %rdx, %rdx
> > > > > > > >     jle     L(zero_len)
> > > > > > > > ```
> > > > > > > >
> > > > > > > > 2) Length check in page-cross case checking if we should continue is
> > > > > > > > doing:
> > > > > > > > ```
> > > > > > > >     cmpq    %r8, %rdx
> > > > > > > >     jb      L(page_cross_small)
> > > > > > > > ```
> > > > > > > > which means we will continue searching for null-term if length ends at
> > > > > > > > the end of a page and there was no null-term in `src`.
> > > > > > >
> > > > > > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > > > > > In this case, src may point to PROT_NONE memory.
> > > > > > >
> > > > > >
> > > > > > If there was a null-term the later check:
> > > > > > ```
> > > > > > shll $CHAR_SIZE, %ecx
> > > > > > jz L(page_cross_continue)
> > > > > > ```
> > > > > > would catch it, this can only occur if there is no null-term.
> > > > >
> > > > > But it is incorrect to read beyond the limit even if there is a null-term.
> > > > It's a page-aligned read where len != 0 so the original read should be fine.
> > > >
> > > > The `jb L(page_cross_small)`
> > > > falls through to the null-term check, so if there was a null-term we won't
> > > > read anymore bytes.
> > >
> > > There is no need to search null-term for zero length case.
> > >
> > This isn't the zero length case, this is the page-cross case. Length
> > is != 0 here.
>
> In the zero length case:
>
>         test    %rdx, %rdx
>         jl      L(zero_len)
>
> it crashes when src points to an unreadable memory and the limit is 0.
> strncat shouldn't read beyond the limit.  The change is OK.   But the
> commit log should mention "avoid reading beyond the limit."

Done in V2.
>
> > > > >
> > > > > > > > The fix is to make the flag:
> > > > > > > > ```
> > > > > > > >     cmpq    %r8, %rdx
> > > > > > > >     jbe     L(page_cross_small)
> > > > > > > > ```
> > > > > > > > ---
> > > > > > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > > > > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > > > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > > > > > index e03d329e1c..c0cde206ee 100644
> > > > > > > > --- a/string/test-strncat.c
> > > > > > > > +++ b/string/test-strncat.c
> > > > > > > > @@ -28,6 +28,7 @@
> > > > > > > >  # define CHAR char
> > > > > > > >  # define UCHAR unsigned char
> > > > > > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > > > > > +# define STRNLEN strnlen
> > > > > > > >  # define STRLEN strlen
> > > > > > > >  # define MEMSET memset
> > > > > > > >  # define MEMCPY memcpy
> > > > > > > > @@ -40,6 +41,7 @@
> > > > > > > >  # define CHAR wchar_t
> > > > > > > >  # define UCHAR wchar_t
> > > > > > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > > > > > +# define STRNLEN wcsnlen
> > > > > > > >  # define STRLEN wcslen
> > > > > > > >  # define MEMSET wmemset
> > > > > > > >  # define MEMCPY wmemcpy
> > > > > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > > >        return;
> > > > > > > >      }
> > > > > > > >
> > > > > > > > -  size_t len = STRLEN (src);
> > > > > > > > +  size_t len = STRNLEN (src, n);
> > > > > > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > > > > > >
> > > > > > > >      {
> > > > > > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > > >      }
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void
> > > > > > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > > > > > +{
> > > > > > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > > > > > +   * bytes." */
> > > > > > > > +  CHAR *s1, *s2;
> > > > > > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > > > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > > > > > +  s2 = (CHAR *) buf2;
> > > > > > > > +  MEMSET (s1 - bound, -1, bound);
> > > > > > > > +  for (size_t n = 0; n < bound; ++n)
> > > > > > > > +    {
> > > > > > > > +      FOR_EACH_IMPL (impl, 0)
> > > > > > > > +       {
> > > > > > > > +         s2[0] = '\0';
> > > > > > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > > > > > +       }
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void
> > > > > > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > > > > > >          size_t n, int max_char)
> > > > > > > > @@ -372,6 +394,7 @@ test_main (void)
> > > > > > > >
> > > > > > > >    do_random_tests ();
> > > > > > > >    do_overflow_tests ();
> > > > > > > > +  do_test_src_no_nullterm_bz30065 ();
> > > > > > > >    return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > > index b380e8e11c..c2ff202238 100644
> > > > > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > > > > > >         salq    $2, %rdx
> > > > > > > >  # else
> > > > > > > >         test    %rdx, %rdx
> > > > > > > > -       jl      L(zero_len)
> > > > > > > > +       jle     L(zero_len)
> > > > > > > >  # endif
> > > > > > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > > > > > >
> > > > > > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > > > > > >         subl    %esi, %r8d
> > > > > > > >         andl    $(VEC_SIZE - 1), %r8d
> > > > > > > >         cmpq    %r8, %rdx
> > > > > > > > -       jb      L(page_cross_small)
> > > > > > > > +       jbe     L(page_cross_small)
> > > > > > > >
> > > > > > > >         /* Optimizing more aggressively for space as this is very cold
> > > > > > > >            code. This saves 2x cache lines.  */
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > H.J.
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > H.J.
> > >
> > >
> > >
> > > --
> > > H.J.
>
>
>
> --
> H.J.
diff mbox series

Patch

diff --git a/string/test-strncat.c b/string/test-strncat.c
index e03d329e1c..c0cde206ee 100644
--- a/string/test-strncat.c
+++ b/string/test-strncat.c
@@ -28,6 +28,7 @@ 
 # define CHAR char
 # define UCHAR unsigned char
 # define SIMPLE_STRNCAT simple_strncat
+# define STRNLEN strnlen
 # define STRLEN strlen
 # define MEMSET memset
 # define MEMCPY memcpy
@@ -40,6 +41,7 @@ 
 # define CHAR wchar_t
 # define UCHAR wchar_t
 # define SIMPLE_STRNCAT simple_wcsncat
+# define STRNLEN wcsnlen
 # define STRLEN wcslen
 # define MEMSET wmemset
 # define MEMCPY wmemcpy
@@ -78,7 +80,7 @@  do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
       return;
     }
 
-  size_t len = STRLEN (src);
+  size_t len = STRNLEN (src, n);
   if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
     {
       error (0, 0, "Incorrect concatenation in function %s",
@@ -95,6 +97,26 @@  do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
     }
 }
 
+static void
+do_test_src_no_nullterm_bz30065 (void)
+{
+  /* NB: "src does not need to be null-terminated if it contains n or more
+   * bytes." */
+  CHAR *s1, *s2;
+  size_t bound = page_size / sizeof (CHAR);
+  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
+  s2 = (CHAR *) buf2;
+  MEMSET (s1 - bound, -1, bound);
+  for (size_t n = 0; n < bound; ++n)
+    {
+      FOR_EACH_IMPL (impl, 0)
+	{
+	  s2[0] = '\0';
+	  do_one_test (impl, s2, s1 - n, n);
+	}
+    }
+}
+
 static void
 do_test (size_t align1, size_t align2, size_t len1, size_t len2,
 	 size_t n, int max_char)
@@ -372,6 +394,7 @@  test_main (void)
 
   do_random_tests ();
   do_overflow_tests ();
+  do_test_src_no_nullterm_bz30065 ();
   return ret;
 }
 
diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
index b380e8e11c..c2ff202238 100644
--- a/sysdeps/x86_64/multiarch/strncat-avx2.S
+++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
@@ -66,7 +66,7 @@  ENTRY(STRNCAT)
 	salq	$2, %rdx
 # else
 	test	%rdx, %rdx
-	jl	L(zero_len)
+	jle	L(zero_len)
 # endif
 	vpxor	%VZERO_128, %VZERO_128, %VZERO_128
 
@@ -387,7 +387,7 @@  L(page_cross):
 	subl	%esi, %r8d
 	andl	$(VEC_SIZE - 1), %r8d
 	cmpq	%r8, %rdx
-	jb	L(page_cross_small)
+	jbe	L(page_cross_small)
 
 	/* Optimizing more aggressively for space as this is very cold
 	   code. This saves 2x cache lines.  */