Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 10/28/2022 07:41, Florian Weimer wrote:
> * Szabolcs Nagy via Libc-alpha:
> 
>  test used scratch_buffer_dupfree incorrectly:
> >
> > - The passed in size must be <= buf.length.
> > - Must be called at most once on a buf object since it frees it.
> > - After it is called buf.data and buf.length must not be accessed.
> >
> > All of these were violated, the test happened to work because the
> > buffer was on the stack, which meant the test copied out-of-bounds
> > bytes from the stack into a new buffer and then compared those bytes.
> >
> > Run one test and avoid the issues above.
> > ---
> >  malloc/tst-scratch_buffer.c | 22 +++++++---------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
> > index 9fcb11ba2c..60a513ccc6 100644
> > --- a/malloc/tst-scratch_buffer.c
> > +++ b/malloc/tst-scratch_buffer.c
> > @@ -155,21 +155,13 @@ do_test (void)
> >      struct scratch_buffer buf;
> >      scratch_buffer_init (&buf);
> >      memset (buf.data, '@', buf.length);
> > -
> > -    size_t sizes[] = { 16, buf.length, buf.length + 16 };
> > -    for (int i = 0; i < array_length (sizes); i++)
> > -      {
> > -        /* The extra size is unitialized through realloc.  */
> > -        size_t l = sizes[i] > buf.length ? sizes[i] : buf.length;
> > -        void *r = scratch_buffer_dupfree (&buf, l);
> > -        void *c = xmalloc (l);
> > -        memset (c, '@', l);
> > -        TEST_COMPARE_BLOB (r, l, buf.data, l);
> > -        free (r);
> > -        free (c);
> > -      }
> > -
> > -    scratch_buffer_free (&buf);
> > +    size_t l = 16 <= buf.length ? 16 : buf.length;
> > +    void *r = scratch_buffer_dupfree (&buf, l);
> > +    void *c = xmalloc (l);
> > +    memset (c, '@', l);
> > +    TEST_COMPARE_BLOB (r, l, c, l);
> > +    free (r);
> > +    free (c);
> >    }
> >    return 0;
> >  }
> 
> I think we should keep the test loop, but create a new scratch buffer on
> each iteration.

given the documentation of scratch_buffer_dupfree
i don't see how the test supposed to work with
sizes > buf.length or what's the point of this loop.