diff mbox series

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

Message ID b4ecb76a1c47dfac8344706e365686bd2620affe.1666877952.git.szabolcs.nagy@arm.com
State New
Headers show
Series patches from the morello port | expand

Commit Message

Szabolcs Nagy Oct. 27, 2022, 3:33 p.m. UTC 
The 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(-)

Comments

Florian Weimer Oct. 28, 2022, 5:41 a.m. UTC | #1 
* 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.

Thanks,
Florian
Szabolcs Nagy Oct. 28, 2022, 11:24 a.m. UTC | #2 
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.
Florian Weimer Oct. 28, 2022, 11:30 a.m. UTC | #3 
* Szabolcs Nagy:

>> 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.

Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
patch, or do you want to do it?

Thanks,
Florian
Szabolcs Nagy Oct. 28, 2022, 12:23 p.m. UTC | #4 
The 10/28/2022 13:30, Florian Weimer via Libc-alpha wrote:
> * Szabolcs Nagy:
> 
> >> 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.
> 
> Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
> patch, or do you want to do it?

i think my original patch makes sense that at least has
one scratch_buffer_dupfree test.

or do you prefer to remove this bit completely?
Florian Weimer Oct. 28, 2022, 12:27 p.m. UTC | #5 
* Szabolcs Nagy:

> The 10/28/2022 13:30, Florian Weimer via Libc-alpha wrote:
>> * Szabolcs Nagy:
>> 
>> >> 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.
>> 
>> Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
>> patch, or do you want to do it?
>
> i think my original patch makes sense that at least has
> one scratch_buffer_dupfree test.
>
> or do you prefer to remove this bit completely?

Sorry I meant we should remove scratch_buffer_dupfree along with its
test because it's unused after commit
ef0700004bf0dccf493a5e8e21f71d9e7972ea9f ("stdlib: Sync canonicalize
with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]").

Thanks,
Florian
diff mbox series

Patch

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;
 }