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

[Noah Goldstein]

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