diff mbox series

[v1] String: Strength memset tests in test-memset.c

Message ID 20220214014037.2422450-1-goldstein.w.n@gmail.com
State Committed
Commit 0281c7a7ec8f3f46d8e6f5f3d7fca548946dbfce
Headers
Series [v1] String: Strength memset tests in test-memset.c |

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

Noah Goldstein Feb. 14, 2022, 1:40 a.m. UTC 
  The prior sentinel logic was broken and was checking the SIMPLE_MEMSET
as opposed to the tested implementation. As well `s` (the test buffer)
was not reset between implementation tests so it was possible for a
buggy implementation to be hidden by a previously executed correct
one.
---
 string/test-memset.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Comments

Siddhesh Poyarekar Feb. 15, 2022, 1:52 p.m. UTC | #1 
On 14/02/2022 07:10, Noah Goldstein via Libc-alpha wrote:
> The prior sentinel logic was broken and was checking the SIMPLE_MEMSET
> as opposed to the tested implementation. As well `s` (the test buffer)
> was not reset between implementation tests so it was possible for a
> buggy implementation to be hidden by a previously executed correct
> one.
> ---
>   string/test-memset.c | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/string/test-memset.c b/string/test-memset.c
> index 8498b1bc97..ee548f6924 100644
> --- a/string/test-memset.c
> +++ b/string/test-memset.c
> @@ -106,26 +106,28 @@ SIMPLE_MEMSET (CHAR *s, int c, size_t n)
>   }
>   
>   static void
> -do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
> +do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above)
>   {
> -  CHAR buf[n + 2];
> -  CHAR *tstbuf = buf + 1;
> -  CHAR sentinel = c - 1;
> -  buf[0] = sentinel;
> -  buf[n + 1] = sentinel;
> +  CHAR buf[n];
> +  CHAR sentinel = ~c;
> +  if (space_below)
> +      s[-1] = sentinel;
> +  if (space_above)
> +      s[n] = sentinel;
> +  SIMPLE_MEMSET(s, ~c, n);

Setting s with ~c...

>   #ifdef TEST_BZERO
> -  simple_bzero (tstbuf, n);
> +  simple_bzero (buf, n);
>     CALL (impl, s, n);
> -  if (memcmp (s, tstbuf, n) != 0
> -      || buf[0] != sentinel
> -      || buf[n + 1] != sentinel)
> +  if (memcmp (s, buf, n) != 0
> +      || (space_below && s[-1] != sentinel)
> +      || (space_above && s[n] != sentinel))
>   #else
>     CHAR *res = CALL (impl, s, c, n);

... and then overwriting it with c...

>     if (res != s
> -      || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf
> -      || MEMCMP (s, tstbuf, n) != 0
> -      || buf[0] != sentinel
> -      || buf[n + 1] != sentinel)
> +      || SIMPLE_MEMSET (buf, c, n) != buf
> +      || MEMCMP (s, buf, n) != 0

... which should then equate to buf since it is also set to c.  OK.

> +      || (space_below && s[-1] != sentinel)
> +      || (space_above && s[n] != sentinel))
>   #endif /* !TEST_BZERO */
>       {
>         error (0, 0, "Wrong result in function %s", impl->name);
> @@ -137,12 +139,16 @@ do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
>   static void
>   do_test (size_t align, int c, size_t len)
>   {
> +  int space_below, space_above;
>     align &= 4095;
>     if ((align + len) * sizeof (CHAR) > page_size)
>       return;
>   
> +  space_below = !!align;
> +  space_above = !((align + len + 1) * sizeof (CHAR) > page_size);
> +
>     FOR_EACH_IMPL (impl, 0)
> -    do_one_test (impl, (CHAR *) (buf1) + align, c, len);
> +    do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above);

This would reduce test coverage, testing underflow only in cases where 
align is non-zero.  Couldn't this be fixed without this coverage reduction?

Thanks,
Siddhesh
Noah Goldstein Feb. 15, 2022, 1:55 p.m. UTC | #2 
On Tue, Feb 15, 2022 at 7:52 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 14/02/2022 07:10, Noah Goldstein via Libc-alpha wrote:
> > The prior sentinel logic was broken and was checking the SIMPLE_MEMSET
> > as opposed to the tested implementation. As well `s` (the test buffer)
> > was not reset between implementation tests so it was possible for a
> > buggy implementation to be hidden by a previously executed correct
> > one.
> > ---
> >   string/test-memset.c | 36 +++++++++++++++++++++---------------
> >   1 file changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/string/test-memset.c b/string/test-memset.c
> > index 8498b1bc97..ee548f6924 100644
> > --- a/string/test-memset.c
> > +++ b/string/test-memset.c
> > @@ -106,26 +106,28 @@ SIMPLE_MEMSET (CHAR *s, int c, size_t n)
> >   }
> >
> >   static void
> > -do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
> > +do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above)
> >   {
> > -  CHAR buf[n + 2];
> > -  CHAR *tstbuf = buf + 1;
> > -  CHAR sentinel = c - 1;
> > -  buf[0] = sentinel;
> > -  buf[n + 1] = sentinel;
> > +  CHAR buf[n];
> > +  CHAR sentinel = ~c;
> > +  if (space_below)
> > +      s[-1] = sentinel;
> > +  if (space_above)
> > +      s[n] = sentinel;
> > +  SIMPLE_MEMSET(s, ~c, n);
>
> Setting s with ~c...
>
> >   #ifdef TEST_BZERO
> > -  simple_bzero (tstbuf, n);
> > +  simple_bzero (buf, n);
> >     CALL (impl, s, n);
> > -  if (memcmp (s, tstbuf, n) != 0
> > -      || buf[0] != sentinel
> > -      || buf[n + 1] != sentinel)
> > +  if (memcmp (s, buf, n) != 0
> > +      || (space_below && s[-1] != sentinel)
> > +      || (space_above && s[n] != sentinel))
> >   #else
> >     CHAR *res = CALL (impl, s, c, n);
>
> ... and then overwriting it with c...
>
> >     if (res != s
> > -      || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf
> > -      || MEMCMP (s, tstbuf, n) != 0
> > -      || buf[0] != sentinel
> > -      || buf[n + 1] != sentinel)
> > +      || SIMPLE_MEMSET (buf, c, n) != buf
> > +      || MEMCMP (s, buf, n) != 0
>
> ... which should then equate to buf since it is also set to c.  OK.
>
> > +      || (space_below && s[-1] != sentinel)
> > +      || (space_above && s[n] != sentinel))
> >   #endif /* !TEST_BZERO */
> >       {
> >         error (0, 0, "Wrong result in function %s", impl->name);
> > @@ -137,12 +139,16 @@ do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
> >   static void
> >   do_test (size_t align, int c, size_t len)
> >   {
> > +  int space_below, space_above;
> >     align &= 4095;
> >     if ((align + len) * sizeof (CHAR) > page_size)
> >       return;
> >
> > +  space_below = !!align;
> > +  space_above = !((align + len + 1) * sizeof (CHAR) > page_size);
> > +
> >     FOR_EACH_IMPL (impl, 0)
> > -    do_one_test (impl, (CHAR *) (buf1) + align, c, len);
> > +    do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above);
>
> This would reduce test coverage, testing underflow only in cases where
> align is non-zero.  Couldn't this be fixed without this coverage reduction?

It doesn't reduce test coverage. As the commit message says the previous
sentinel logic was working on `tstbuf` so for all intents and purposes we were
entirely missing it before.

I think also if align == 0 we are at the beginning of a page and will segfault
if we have underflow.
>
> Thanks,
> Siddhesh
Siddhesh Poyarekar Feb. 15, 2022, 2 p.m. UTC | #3 
On 15/02/2022 19:25, Noah Goldstein wrote:
> On Tue, Feb 15, 2022 at 7:52 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>>
>> On 14/02/2022 07:10, Noah Goldstein via Libc-alpha wrote:
>>> The prior sentinel logic was broken and was checking the SIMPLE_MEMSET
>>> as opposed to the tested implementation. As well `s` (the test buffer)
>>> was not reset between implementation tests so it was possible for a
>>> buggy implementation to be hidden by a previously executed correct
>>> one.
>>> ---
>>>    string/test-memset.c | 36 +++++++++++++++++++++---------------
>>>    1 file changed, 21 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/string/test-memset.c b/string/test-memset.c
>>> index 8498b1bc97..ee548f6924 100644
>>> --- a/string/test-memset.c
>>> +++ b/string/test-memset.c
>>> @@ -106,26 +106,28 @@ SIMPLE_MEMSET (CHAR *s, int c, size_t n)
>>>    }
>>>
>>>    static void
>>> -do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
>>> +do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above)
>>>    {
>>> -  CHAR buf[n + 2];
>>> -  CHAR *tstbuf = buf + 1;
>>> -  CHAR sentinel = c - 1;
>>> -  buf[0] = sentinel;
>>> -  buf[n + 1] = sentinel;
>>> +  CHAR buf[n];
>>> +  CHAR sentinel = ~c;
>>> +  if (space_below)
>>> +      s[-1] = sentinel;
>>> +  if (space_above)
>>> +      s[n] = sentinel;
>>> +  SIMPLE_MEMSET(s, ~c, n);
>>
>> Setting s with ~c...
>>
>>>    #ifdef TEST_BZERO
>>> -  simple_bzero (tstbuf, n);
>>> +  simple_bzero (buf, n);
>>>      CALL (impl, s, n);
>>> -  if (memcmp (s, tstbuf, n) != 0
>>> -      || buf[0] != sentinel
>>> -      || buf[n + 1] != sentinel)
>>> +  if (memcmp (s, buf, n) != 0
>>> +      || (space_below && s[-1] != sentinel)
>>> +      || (space_above && s[n] != sentinel))
>>>    #else
>>>      CHAR *res = CALL (impl, s, c, n);
>>
>> ... and then overwriting it with c...
>>
>>>      if (res != s
>>> -      || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf
>>> -      || MEMCMP (s, tstbuf, n) != 0
>>> -      || buf[0] != sentinel
>>> -      || buf[n + 1] != sentinel)
>>> +      || SIMPLE_MEMSET (buf, c, n) != buf
>>> +      || MEMCMP (s, buf, n) != 0
>>
>> ... which should then equate to buf since it is also set to c.  OK.
>>
>>> +      || (space_below && s[-1] != sentinel)
>>> +      || (space_above && s[n] != sentinel))
>>>    #endif /* !TEST_BZERO */
>>>        {
>>>          error (0, 0, "Wrong result in function %s", impl->name);
>>> @@ -137,12 +139,16 @@ do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
>>>    static void
>>>    do_test (size_t align, int c, size_t len)
>>>    {
>>> +  int space_below, space_above;
>>>      align &= 4095;
>>>      if ((align + len) * sizeof (CHAR) > page_size)
>>>        return;
>>>
>>> +  space_below = !!align;
>>> +  space_above = !((align + len + 1) * sizeof (CHAR) > page_size);
>>> +
>>>      FOR_EACH_IMPL (impl, 0)
>>> -    do_one_test (impl, (CHAR *) (buf1) + align, c, len);
>>> +    do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above);
>>
>> This would reduce test coverage, testing underflow only in cases where
>> align is non-zero.  Couldn't this be fixed without this coverage reduction?
> 
> It doesn't reduce test coverage. As the commit message says the previous
> sentinel logic was working on `tstbuf` so for all intents and purposes we were
> entirely missing it before.
> 
> I think also if align == 0 we are at the beginning of a page and will segfault
> if we have underflow.

Of course, I had a memory of buf1 and buf2 being sub-buffers of a larger 
buffer, but I clearly misremembered.

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Noah Goldstein Feb. 15, 2022, 5:41 p.m. UTC | #4 
On Tue, Feb 15, 2022 at 8:01 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 15/02/2022 19:25, Noah Goldstein wrote:
> > On Tue, Feb 15, 2022 at 7:52 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> >>
> >> On 14/02/2022 07:10, Noah Goldstein via Libc-alpha wrote:
> >>> The prior sentinel logic was broken and was checking the SIMPLE_MEMSET
> >>> as opposed to the tested implementation. As well `s` (the test buffer)
> >>> was not reset between implementation tests so it was possible for a
> >>> buggy implementation to be hidden by a previously executed correct
> >>> one.
> >>> ---
> >>>    string/test-memset.c | 36 +++++++++++++++++++++---------------
> >>>    1 file changed, 21 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/string/test-memset.c b/string/test-memset.c
> >>> index 8498b1bc97..ee548f6924 100644
> >>> --- a/string/test-memset.c
> >>> +++ b/string/test-memset.c
> >>> @@ -106,26 +106,28 @@ SIMPLE_MEMSET (CHAR *s, int c, size_t n)
> >>>    }
> >>>
> >>>    static void
> >>> -do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
> >>> +do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above)
> >>>    {
> >>> -  CHAR buf[n + 2];
> >>> -  CHAR *tstbuf = buf + 1;
> >>> -  CHAR sentinel = c - 1;
> >>> -  buf[0] = sentinel;
> >>> -  buf[n + 1] = sentinel;
> >>> +  CHAR buf[n];
> >>> +  CHAR sentinel = ~c;
> >>> +  if (space_below)
> >>> +      s[-1] = sentinel;
> >>> +  if (space_above)
> >>> +      s[n] = sentinel;
> >>> +  SIMPLE_MEMSET(s, ~c, n);
> >>
> >> Setting s with ~c...
> >>
> >>>    #ifdef TEST_BZERO
> >>> -  simple_bzero (tstbuf, n);
> >>> +  simple_bzero (buf, n);
> >>>      CALL (impl, s, n);
> >>> -  if (memcmp (s, tstbuf, n) != 0
> >>> -      || buf[0] != sentinel
> >>> -      || buf[n + 1] != sentinel)
> >>> +  if (memcmp (s, buf, n) != 0
> >>> +      || (space_below && s[-1] != sentinel)
> >>> +      || (space_above && s[n] != sentinel))
> >>>    #else
> >>>      CHAR *res = CALL (impl, s, c, n);
> >>
> >> ... and then overwriting it with c...
> >>
> >>>      if (res != s
> >>> -      || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf
> >>> -      || MEMCMP (s, tstbuf, n) != 0
> >>> -      || buf[0] != sentinel
> >>> -      || buf[n + 1] != sentinel)
> >>> +      || SIMPLE_MEMSET (buf, c, n) != buf
> >>> +      || MEMCMP (s, buf, n) != 0
> >>
> >> ... which should then equate to buf since it is also set to c.  OK.
> >>
> >>> +      || (space_below && s[-1] != sentinel)
> >>> +      || (space_above && s[n] != sentinel))
> >>>    #endif /* !TEST_BZERO */
> >>>        {
> >>>          error (0, 0, "Wrong result in function %s", impl->name);
> >>> @@ -137,12 +139,16 @@ do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
> >>>    static void
> >>>    do_test (size_t align, int c, size_t len)
> >>>    {
> >>> +  int space_below, space_above;
> >>>      align &= 4095;
> >>>      if ((align + len) * sizeof (CHAR) > page_size)
> >>>        return;
> >>>
> >>> +  space_below = !!align;
> >>> +  space_above = !((align + len + 1) * sizeof (CHAR) > page_size);
> >>> +
> >>>      FOR_EACH_IMPL (impl, 0)
> >>> -    do_one_test (impl, (CHAR *) (buf1) + align, c, len);
> >>> +    do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above);
> >>
> >> This would reduce test coverage, testing underflow only in cases where
> >> align is non-zero.  Couldn't this be fixed without this coverage reduction?
> >
> > It doesn't reduce test coverage. As the commit message says the previous
> > sentinel logic was working on `tstbuf` so for all intents and purposes we were
> > entirely missing it before.
> >
> > I think also if align == 0 we are at the beginning of a page and will segfault
> > if we have underflow.
>
> Of course, I had a memory of buf1 and buf2 being sub-buffers of a larger
> buffer, but I clearly misremembered.
>
> LGTM.
>
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Thanks pushed.
>
diff mbox series

Patch

diff --git a/string/test-memset.c b/string/test-memset.c
index 8498b1bc97..ee548f6924 100644
--- a/string/test-memset.c
+++ b/string/test-memset.c
@@ -106,26 +106,28 @@  SIMPLE_MEMSET (CHAR *s, int c, size_t n)
 }
 
 static void
-do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
+do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above)
 {
-  CHAR buf[n + 2];
-  CHAR *tstbuf = buf + 1;
-  CHAR sentinel = c - 1;
-  buf[0] = sentinel;
-  buf[n + 1] = sentinel;
+  CHAR buf[n];
+  CHAR sentinel = ~c;
+  if (space_below)
+      s[-1] = sentinel;
+  if (space_above)
+      s[n] = sentinel;
+  SIMPLE_MEMSET(s, ~c, n);
 #ifdef TEST_BZERO
-  simple_bzero (tstbuf, n);
+  simple_bzero (buf, n);
   CALL (impl, s, n);
-  if (memcmp (s, tstbuf, n) != 0
-      || buf[0] != sentinel
-      || buf[n + 1] != sentinel)
+  if (memcmp (s, buf, n) != 0
+      || (space_below && s[-1] != sentinel)
+      || (space_above && s[n] != sentinel))
 #else
   CHAR *res = CALL (impl, s, c, n);
   if (res != s
-      || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf
-      || MEMCMP (s, tstbuf, n) != 0
-      || buf[0] != sentinel
-      || buf[n + 1] != sentinel)
+      || SIMPLE_MEMSET (buf, c, n) != buf
+      || MEMCMP (s, buf, n) != 0
+      || (space_below && s[-1] != sentinel)
+      || (space_above && s[n] != sentinel))
 #endif /* !TEST_BZERO */
     {
       error (0, 0, "Wrong result in function %s", impl->name);
@@ -137,12 +139,16 @@  do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n)
 static void
 do_test (size_t align, int c, size_t len)
 {
+  int space_below, space_above;
   align &= 4095;
   if ((align + len) * sizeof (CHAR) > page_size)
     return;
 
+  space_below = !!align;
+  space_above = !((align + len + 1) * sizeof (CHAR) > page_size);
+
   FOR_EACH_IMPL (impl, 0)
-    do_one_test (impl, (CHAR *) (buf1) + align, c, len);
+    do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above);
 }
 
 #ifndef TEST_BZERO