x86: Add a testcase for BZ #29863
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
When a thread is updating the memory area of memcmp and another thread
is doing memcmp, the memcmp return value is undefined. Add a testcase
to verify that memcmp won't segfault in this case.
---
sysdeps/x86/Makefile | 11 +++++
sysdeps/x86/tst-memcmp-race-sse2.c | 1 +
sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++
3 files changed, 87 insertions(+)
create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c
create mode 100644 sysdeps/x86/tst-memcmp-race.c
Comments
On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> When a thread is updating the memory area of memcmp and another thread
> is doing memcmp, the memcmp return value is undefined. Add a testcase
> to verify that memcmp won't segfault in this case.
> ---
> sysdeps/x86/Makefile | 11 +++++
> sysdeps/x86/tst-memcmp-race-sse2.c | 1 +
> sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++
> 3 files changed, 87 insertions(+)
> create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c
> create mode 100644 sysdeps/x86/tst-memcmp-race.c
>
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index 56fd5fc805..3e07e18c7d 100644
> --- a/sysdeps/x86/Makefile
> +++ b/sysdeps/x86/Makefile
> @@ -257,3 +257,14 @@ tests += \
> tests-static += \
> tst-sysconf-cache-linesize-static
> endif
> +
> +ifeq ($(subdir),nptl)
> +tests += \
> + tst-memcmp-race-sse2 \
> + tst-memcmp-race \
> +# tests
> +
> +CFLAGS-tst-memcmp-race-sse2.c += -O0
> +CFLAGS-tst-memcmp-race.c += -O0
> +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2
> +endif
> diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c
> new file mode 100644
> index 0000000000..2986e418f3
> --- /dev/null
> +++ b/sysdeps/x86/tst-memcmp-race-sse2.c
> @@ -0,0 +1 @@
> +#include "tst-memcmp-race.c"
> diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c
> new file mode 100644
> index 0000000000..f8f8e47f6e
> --- /dev/null
> +++ b/sysdeps/x86/tst-memcmp-race.c
> @@ -0,0 +1,75 @@
> +/* Test case for memcmp with race condition.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <support/xthread.h>
> +
> +#define NUM_THREADS 2
> +#define LEN 17
Imo if we test this `LEN` should be a variable and we should test
all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are
other cases.
> +#define STR "abcdefghijklmnopq"
> +#define LOOP1 100
> +#define LOOP2 1000000
> +
> +char buf1[LEN], buf2[LEN];
I had to make this volatile to reproduce the failure
with GCC 12, does this properly segfault w.o the
fix patch?
> +
> +static void *
> +childThread (void *tArgs)
> +{
> + int i;
> +
> + if (0 == (size_t)tArgs % 2)
> + {
> + for (i = 0; i < LOOP1; i++)
> + {
> + int result;
> +
> + result = memcmp (buf1, buf2, LEN);
If we are testing this I think it should test each impl.
> + printf ("i = %d : result = %d\n", i, result);
IMO remove the print and make `result` volatile.
> + }
> + }
> + else
> + {
> + for (i = 0; i < LOOP2; i++)
> + buf1[1] = (0 == (i % 2)) ? 'b' : 'c';
> + }
> +
> + return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> + int i;
> + pthread_t threads[NUM_THREADS];
> +
> + memcpy (buf1, STR, LEN);
> + memcpy (buf2, STR, LEN);
> +
> + for (i = 0; i < NUM_THREADS; ++i)
> + threads[i] = xpthread_create (NULL, childThread,
> + (void *)(uintptr_t) i);
> +
> + for (i = 0; i < NUM_THREADS; ++i)
> + xpthread_join (threads[i]);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> --
> 2.38.1
>
Not sure we want this, given its not behavior we have decided
to actively support its failures could be misleading.
On Wed, Dec 14, 2022 at 9:45 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > When a thread is updating the memory area of memcmp and another thread
> > is doing memcmp, the memcmp return value is undefined. Add a testcase
> > to verify that memcmp won't segfault in this case.
> > ---
> > sysdeps/x86/Makefile | 11 +++++
> > sysdeps/x86/tst-memcmp-race-sse2.c | 1 +
> > sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++
> > 3 files changed, 87 insertions(+)
> > create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c
> > create mode 100644 sysdeps/x86/tst-memcmp-race.c
> >
> > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> > index 56fd5fc805..3e07e18c7d 100644
> > --- a/sysdeps/x86/Makefile
> > +++ b/sysdeps/x86/Makefile
> > @@ -257,3 +257,14 @@ tests += \
> > tests-static += \
> > tst-sysconf-cache-linesize-static
> > endif
> > +
> > +ifeq ($(subdir),nptl)
> > +tests += \
> > + tst-memcmp-race-sse2 \
> > + tst-memcmp-race \
> > +# tests
> > +
> > +CFLAGS-tst-memcmp-race-sse2.c += -O0
> > +CFLAGS-tst-memcmp-race.c += -O0
> > +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2
> > +endif
> > diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c
> > new file mode 100644
> > index 0000000000..2986e418f3
> > --- /dev/null
> > +++ b/sysdeps/x86/tst-memcmp-race-sse2.c
> > @@ -0,0 +1 @@
> > +#include "tst-memcmp-race.c"
> > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c
> > new file mode 100644
> > index 0000000000..f8f8e47f6e
> > --- /dev/null
> > +++ b/sysdeps/x86/tst-memcmp-race.c
> > @@ -0,0 +1,75 @@
> > +/* Test case for memcmp with race condition.
> > + Copyright (C) 2022 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > +
> > + The GNU C Library is free software; you can redistribute it and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later version.
> > +
> > + The GNU C Library is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with the GNU C Library; if not, see
> > + <https://www.gnu.org/licenses/>. */
> > +
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <support/xthread.h>
> > +
> > +#define NUM_THREADS 2
> > +#define LEN 17
>
> Imo if we test this `LEN` should be a variable and we should test
> all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are
> other cases.
Fixd in v2.
> > +#define STR "abcdefghijklmnopq"
> > +#define LOOP1 100
> > +#define LOOP2 1000000
> > +
> > +char buf1[LEN], buf2[LEN];
>
> I had to make this volatile to reproduce the failure
> with GCC 12, does this properly segfault w.o the
> fix patch?
Yes, with -O0.
> > +
> > +static void *
> > +childThread (void *tArgs)
> > +{
> > + int i;
> > +
> > + if (0 == (size_t)tArgs % 2)
> > + {
> > + for (i = 0; i < LOOP1; i++)
> > + {
> > + int result;
> > +
> > + result = memcmp (buf1, buf2, LEN);
>
> If we are testing this I think it should test each impl.
Fixed in v2.
> > + printf ("i = %d : result = %d\n", i, result);
>
> IMO remove the print and make `result` volatile.
There is a race condition. -O0 can reproduce it with GCC 12.
> > + }
> > + }
> > + else
> > + {
> > + for (i = 0; i < LOOP2; i++)
> > + buf1[1] = (0 == (i % 2)) ? 'b' : 'c';
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > + int i;
> > + pthread_t threads[NUM_THREADS];
> > +
> > + memcpy (buf1, STR, LEN);
> > + memcpy (buf2, STR, LEN);
> > +
> > + for (i = 0; i < NUM_THREADS; ++i)
> > + threads[i] = xpthread_create (NULL, childThread,
> > + (void *)(uintptr_t) i);
> > +
> > + for (i = 0; i < NUM_THREADS; ++i)
> > + xpthread_join (threads[i]);
> > +
> > + return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
> > --
> > 2.38.1
> >
>
> Not sure we want this, given its not behavior we have decided
> to actively support its failures could be misleading.
This testcase only checks segfault, not memcmp return values.
On Wed, Dec 14, 2022 at 11:49 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Dec 14, 2022 at 9:45 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > When a thread is updating the memory area of memcmp and another thread
> > > is doing memcmp, the memcmp return value is undefined. Add a testcase
> > > to verify that memcmp won't segfault in this case.
> > > ---
> > > sysdeps/x86/Makefile | 11 +++++
> > > sysdeps/x86/tst-memcmp-race-sse2.c | 1 +
> > > sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++
> > > 3 files changed, 87 insertions(+)
> > > create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c
> > > create mode 100644 sysdeps/x86/tst-memcmp-race.c
> > >
> > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> > > index 56fd5fc805..3e07e18c7d 100644
> > > --- a/sysdeps/x86/Makefile
> > > +++ b/sysdeps/x86/Makefile
> > > @@ -257,3 +257,14 @@ tests += \
> > > tests-static += \
> > > tst-sysconf-cache-linesize-static
> > > endif
> > > +
> > > +ifeq ($(subdir),nptl)
> > > +tests += \
> > > + tst-memcmp-race-sse2 \
> > > + tst-memcmp-race \
> > > +# tests
> > > +
> > > +CFLAGS-tst-memcmp-race-sse2.c += -O0
> > > +CFLAGS-tst-memcmp-race.c += -O0
> > > +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2
> > > +endif
> > > diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c
> > > new file mode 100644
> > > index 0000000000..2986e418f3
> > > --- /dev/null
> > > +++ b/sysdeps/x86/tst-memcmp-race-sse2.c
> > > @@ -0,0 +1 @@
> > > +#include "tst-memcmp-race.c"
> > > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c
> > > new file mode 100644
> > > index 0000000000..f8f8e47f6e
> > > --- /dev/null
> > > +++ b/sysdeps/x86/tst-memcmp-race.c
> > > @@ -0,0 +1,75 @@
> > > +/* Test case for memcmp with race condition.
> > > + Copyright (C) 2022 Free Software Foundation, Inc.
> > > + This file is part of the GNU C Library.
> > > +
> > > + The GNU C Library is free software; you can redistribute it and/or
> > > + modify it under the terms of the GNU Lesser General Public
> > > + License as published by the Free Software Foundation; either
> > > + version 2.1 of the License, or (at your option) any later version.
> > > +
> > > + The GNU C Library is distributed in the hope that it will be useful,
> > > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + Lesser General Public License for more details.
> > > +
> > > + You should have received a copy of the GNU Lesser General Public
> > > + License along with the GNU C Library; if not, see
> > > + <https://www.gnu.org/licenses/>. */
> > > +
> > > +#include <stdio.h>
> > > +#include <stdint.h>
> > > +#include <string.h>
> > > +#include <support/xthread.h>
> > > +
> > > +#define NUM_THREADS 2
> > > +#define LEN 17
> >
> > Imo if we test this `LEN` should be a variable and we should test
> > all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are
> > other cases.
>
> Fixd in v2.
>
> > > +#define STR "abcdefghijklmnopq"
> > > +#define LOOP1 100
> > > +#define LOOP2 1000000
> > > +
> > > +char buf1[LEN], buf2[LEN];
> >
> > I had to make this volatile to reproduce the failure
> > with GCC 12, does this properly segfault w.o the
> > fix patch?
>
> Yes, with -O0.
>
> > > +
> > > +static void *
> > > +childThread (void *tArgs)
> > > +{
> > > + int i;
> > > +
> > > + if (0 == (size_t)tArgs % 2)
> > > + {
> > > + for (i = 0; i < LOOP1; i++)
> > > + {
> > > + int result;
> > > +
> > > + result = memcmp (buf1, buf2, LEN);
> >
> > If we are testing this I think it should test each impl.
>
> Fixed in v2.
>
> > > + printf ("i = %d : result = %d\n", i, result);
> >
> > IMO remove the print and make `result` volatile.
>
> There is a race condition. -O0 can reproduce it with GCC 12.
>
> > > + }
> > > + }
> > > + else
> > > + {
> > > + for (i = 0; i < LOOP2; i++)
> > > + buf1[1] = (0 == (i % 2)) ? 'b' : 'c';
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +static int
> > > +do_test (void)
> > > +{
> > > + int i;
> > > + pthread_t threads[NUM_THREADS];
> > > +
> > > + memcpy (buf1, STR, LEN);
> > > + memcpy (buf2, STR, LEN);
> > > +
> > > + for (i = 0; i < NUM_THREADS; ++i)
> > > + threads[i] = xpthread_create (NULL, childThread,
> > > + (void *)(uintptr_t) i);
> > > +
> > > + for (i = 0; i < NUM_THREADS; ++i)
> > > + xpthread_join (threads[i]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#include <support/test-driver.c>
> > > --
> > > 2.38.1
> > >
> >
> > Not sure we want this, given its not behavior we have decided
> > to actively support its failures could be misleading.
>
> This testcase only checks segfault, not memcmp return values.
At this point I don't think we have committed to not segfaulting
in the data-race case.
>
> --
> H.J.
@@ -257,3 +257,14 @@ tests += \
tests-static += \
tst-sysconf-cache-linesize-static
endif
+
+ifeq ($(subdir),nptl)
+tests += \
+ tst-memcmp-race-sse2 \
+ tst-memcmp-race \
+# tests
+
+CFLAGS-tst-memcmp-race-sse2.c += -O0
+CFLAGS-tst-memcmp-race.c += -O0
+tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2
+endif
new file mode 100644
@@ -0,0 +1 @@
+#include "tst-memcmp-race.c"
new file mode 100644
@@ -0,0 +1,75 @@
+/* Test case for memcmp with race condition.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+#include <support/xthread.h>
+
+#define NUM_THREADS 2
+#define LEN 17
+#define STR "abcdefghijklmnopq"
+#define LOOP1 100
+#define LOOP2 1000000
+
+char buf1[LEN], buf2[LEN];
+
+static void *
+childThread (void *tArgs)
+{
+ int i;
+
+ if (0 == (size_t)tArgs % 2)
+ {
+ for (i = 0; i < LOOP1; i++)
+ {
+ int result;
+
+ result = memcmp (buf1, buf2, LEN);
+ printf ("i = %d : result = %d\n", i, result);
+ }
+ }
+ else
+ {
+ for (i = 0; i < LOOP2; i++)
+ buf1[1] = (0 == (i % 2)) ? 'b' : 'c';
+ }
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ int i;
+ pthread_t threads[NUM_THREADS];
+
+ memcpy (buf1, STR, LEN);
+ memcpy (buf2, STR, LEN);
+
+ for (i = 0; i < NUM_THREADS; ++i)
+ threads[i] = xpthread_create (NULL, childThread,
+ (void *)(uintptr_t) i);
+
+ for (i = 0; i < NUM_THREADS; ++i)
+ xpthread_join (threads[i]);
+
+ return 0;
+}
+
+#include <support/test-driver.c>