[PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065]

[Carlos O'Donell]

On 1/31/23 18:46, Noah Goldstein via Libc-alpha wrote:
> Occurs when `src` has no null-term.

This has been pushed as b2c474f8de4c92bfe7435853a96805ec32d68dfa.

We are now in a hard freeze as I prepare to cut the release.

Please do not commit anything further.

If we find other issues we can backport to the release branch after testing.

I'm re-running testing with this patch included for x86_64 and i686.
 
> Two cases:
> 
> 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(-)
> 
> 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.  */

-- 
Cheers,
Carlos.