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

["H.J. Lu" <hjl.tools@gmail.com>]

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
>


-- 
H.J.