* [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] @ 2023-01-31 21:36 Noah Goldstein 2023-01-31 22:28 ` H.J. Lu 2023-01-31 23:46 ` [PATCH v2] x86: Fix strncat-avx2.S reading past length " Noah Goldstein 0 siblings, 2 replies; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 21:36 UTC (permalink / raw) To: libc-alpha; +Cc: goldstein.w.n, hjl.tools, carlos 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(-) 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 21:36 [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] Noah Goldstein @ 2023-01-31 22:28 ` H.J. Lu 2023-01-31 22:37 ` Noah Goldstein 2023-01-31 23:46 ` [PATCH v2] x86: Fix strncat-avx2.S reading past length " Noah Goldstein 1 sibling, 1 reply; 14+ messages in thread From: H.J. Lu @ 2023-01-31 22:28 UTC (permalink / raw) To: Noah Goldstein; +Cc: libc-alpha, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 22:28 ` H.J. Lu @ 2023-01-31 22:37 ` Noah Goldstein 2023-01-31 22:42 ` H.J. Lu 0 siblings, 1 reply; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 22:37 UTC (permalink / raw) To: H.J. Lu; +Cc: libc-alpha, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 22:37 ` Noah Goldstein @ 2023-01-31 22:42 ` H.J. Lu 2023-01-31 22:52 ` Noah Goldstein 0 siblings, 1 reply; 14+ messages in thread From: H.J. Lu @ 2023-01-31 22:42 UTC (permalink / raw) To: Noah Goldstein; +Cc: libc-alpha, carlos 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. -- H.J. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 22:42 ` H.J. Lu @ 2023-01-31 22:52 ` Noah Goldstein 2023-01-31 23:01 ` H.J. Lu 0 siblings, 1 reply; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 22:52 UTC (permalink / raw) To: H.J. Lu; +Cc: libc-alpha, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 22:52 ` Noah Goldstein @ 2023-01-31 23:01 ` H.J. Lu 2023-01-31 23:09 ` Noah Goldstein 0 siblings, 1 reply; 14+ messages in thread From: H.J. Lu @ 2023-01-31 23:01 UTC (permalink / raw) To: Noah Goldstein; +Cc: libc-alpha, carlos 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. -- H.J. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 23:01 ` H.J. Lu @ 2023-01-31 23:09 ` Noah Goldstein 2023-01-31 23:40 ` H.J. Lu 0 siblings, 1 reply; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 23:09 UTC (permalink / raw) To: H.J. Lu; +Cc: libc-alpha, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 23:09 ` Noah Goldstein @ 2023-01-31 23:40 ` H.J. Lu 2023-01-31 23:47 ` Noah Goldstein 0 siblings, 1 reply; 14+ messages in thread From: H.J. Lu @ 2023-01-31 23:40 UTC (permalink / raw) To: Noah Goldstein; +Cc: libc-alpha, carlos 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. -- H.J. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] 2023-01-31 23:40 ` H.J. Lu @ 2023-01-31 23:47 ` Noah Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 23:47 UTC (permalink / raw) To: H.J. Lu; +Cc: libc-alpha, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065] 2023-01-31 21:36 [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] Noah Goldstein 2023-01-31 22:28 ` H.J. Lu @ 2023-01-31 23:46 ` Noah Goldstein 2023-02-01 0:23 ` H.J. Lu 2023-02-01 3:10 ` Carlos O'Donell 1 sibling, 2 replies; 14+ messages in thread From: Noah Goldstein @ 2023-01-31 23:46 UTC (permalink / raw) To: libc-alpha; +Cc: goldstein.w.n, hjl.tools, carlos Occurs when `src` has no null-term. 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. */ -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065] 2023-01-31 23:46 ` [PATCH v2] x86: Fix strncat-avx2.S reading past length " Noah Goldstein @ 2023-02-01 0:23 ` H.J. Lu 2023-02-01 3:10 ` Carlos O'Donell 1 sibling, 0 replies; 14+ messages in thread From: H.J. Lu @ 2023-02-01 0:23 UTC (permalink / raw) To: Noah Goldstein; +Cc: libc-alpha, carlos On Tue, Jan 31, 2023 at 3:47 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > Occurs when `src` has no null-term. > > 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. */ > -- > 2.34.1 > LGTM. Thanks. -- H.J. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065] 2023-01-31 23:46 ` [PATCH v2] x86: Fix strncat-avx2.S reading past length " Noah Goldstein 2023-02-01 0:23 ` H.J. Lu @ 2023-02-01 3:10 ` Carlos O'Donell 2023-02-12 1:00 ` Sunil Pandey 1 sibling, 1 reply; 14+ messages in thread From: Carlos O'Donell @ 2023-02-01 3:10 UTC (permalink / raw) To: Noah Goldstein, libc-alpha; +Cc: hjl.tools, carlos 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065] 2023-02-01 3:10 ` Carlos O'Donell @ 2023-02-12 1:00 ` Sunil Pandey 2023-02-12 1:36 ` H.J. Lu 0 siblings, 1 reply; 14+ messages in thread From: Sunil Pandey @ 2023-02-12 1:00 UTC (permalink / raw) To: Carlos O'Donell, Libc-stable Mailing List Cc: Noah Goldstein, libc-alpha, hjl.tools, carlos On Tue, Jan 31, 2023 at 7:10 PM Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org> wrote: > > 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. > I would like to backport this patch to release branches. Any comments or objections? --Sunil ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86: Fix strncat-avx2.S reading past length [BZ #30065] 2023-02-12 1:00 ` Sunil Pandey @ 2023-02-12 1:36 ` H.J. Lu 0 siblings, 0 replies; 14+ messages in thread From: H.J. Lu @ 2023-02-12 1:36 UTC (permalink / raw) To: Sunil Pandey Cc: Carlos O'Donell, Libc-stable Mailing List, Noah Goldstein, libc-alpha, carlos On Sat, Feb 11, 2023 at 5:00 PM Sunil Pandey <skpgkp2@gmail.com> wrote: > > On Tue, Jan 31, 2023 at 7:10 PM Carlos O'Donell via Libc-alpha > <libc-alpha@sourceware.org> wrote: > > > > 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. > > > > I would like to backport this patch to release branches. > Any comments or objections? > > --Sunil OK. Thanks. -- H.J. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-02-12 1:37 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-31 21:36 [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] Noah Goldstein 2023-01-31 22:28 ` H.J. Lu 2023-01-31 22:37 ` Noah Goldstein 2023-01-31 22:42 ` H.J. Lu 2023-01-31 22:52 ` Noah Goldstein 2023-01-31 23:01 ` H.J. Lu 2023-01-31 23:09 ` Noah Goldstein 2023-01-31 23:40 ` H.J. Lu 2023-01-31 23:47 ` Noah Goldstein 2023-01-31 23:46 ` [PATCH v2] x86: Fix strncat-avx2.S reading past length " Noah Goldstein 2023-02-01 0:23 ` H.J. Lu 2023-02-01 3:10 ` Carlos O'Donell 2023-02-12 1:00 ` Sunil Pandey 2023-02-12 1:36 ` H.J. Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).