* [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
* [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 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
* 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).