[uclibc-ng-devel] malloc: Add missing locks for some paths (valloc/memalign/posix_memalign)
diff mbox series

Message ID 1569398386-5780-1-git-send-email-oftedal@gmail.com
State Accepted
Headers show
Series
  • [uclibc-ng-devel] malloc: Add missing locks for some paths (valloc/memalign/posix_memalign)
Related show

Commit Message

Kjetil Oftedal Sept. 25, 2019, 7:59 a.m. UTC
The internal heap structures were not protected properly in
memalign(). If multiple threads were concurrently allocating memory and
one of them were requesting aligned memory via valloc,memalign or
posix_memalign the internal heap data structures could be corrupted.

Signed-off-by: Kjetil Oftedal <oftedal@gmail.com>
---
 libc/stdlib/malloc/memalign.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Yann Sionneau Sept. 25, 2019, 8:02 a.m. UTC | #1
Hello,

Do you have a small test case that could reproduce the issue that could 
be added to uClibc-ng testsuite?

Thanks!

Yann

On 9/25/19 9:59 AM, Kjetil Oftedal wrote:
> The internal heap structures were not protected properly in
> memalign(). If multiple threads were concurrently allocating memory and
> one of them were requesting aligned memory via valloc,memalign or
> posix_memalign the internal heap data structures could be corrupted.
>
> Signed-off-by: Kjetil Oftedal <oftedal@gmail.com>
> ---
>   libc/stdlib/malloc/memalign.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/libc/stdlib/malloc/memalign.c b/libc/stdlib/malloc/memalign.c
> index 74d5dbd..0d3de67 100644
> --- a/libc/stdlib/malloc/memalign.c
> +++ b/libc/stdlib/malloc/memalign.c
> @@ -77,7 +77,9 @@ memalign (size_t alignment, size_t size)
>   	  init_size = addr - tot_addr;
>   	}
>   
> +      __heap_lock (&__malloc_heap_lock);
>         __heap_free (heap, base, init_size);
> +      __heap_unlock (&__malloc_heap_lock);
>   
>         /* Remember that we've freed the initial part of MEM.  */
>         base += init_size;
> @@ -85,9 +87,11 @@ memalign (size_t alignment, size_t size)
>   
>     /* Return the end part of MEM to the heap, unless it's too small.  */
>     end_addr = addr + size;
> -  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr)
> +  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr) {
> +    __heap_lock (&__malloc_heap_lock);
>       __heap_free (heap, (void *)end_addr, tot_end_addr - end_addr);
> -  else
> +    __heap_unlock (&__malloc_heap_lock);
> +  } else
>       /* We didn't free the end, so include it in the size.  */
>       end_addr = tot_end_addr;
>
Kjetil Oftedal Sept. 25, 2019, 3:01 p.m. UTC | #2
Hi,

Unfortunately, no. It was triggered by a proprietary userland application.
And since it is a concurrency problem it can be difficult to create a
reliable test.

I guess one could just hammer malloc() and memalign() with a bunch of pthreads,
and see if the application crashes.

Best regards,
Kjetil Oftedal

On Wed, 25 Sep 2019 at 10:02, Yann Sionneau <ysionneau@kalray.eu> wrote:
>
> Hello,
>
> Do you have a small test case that could reproduce the issue that could
> be added to uClibc-ng testsuite?
>
> Thanks!
>
> Yann
>
> On 9/25/19 9:59 AM, Kjetil Oftedal wrote:
> > The internal heap structures were not protected properly in
> > memalign(). If multiple threads were concurrently allocating memory and
> > one of them were requesting aligned memory via valloc,memalign or
> > posix_memalign the internal heap data structures could be corrupted.
> >
> > Signed-off-by: Kjetil Oftedal <oftedal@gmail.com>
> > ---
> >   libc/stdlib/malloc/memalign.c |    8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/libc/stdlib/malloc/memalign.c b/libc/stdlib/malloc/memalign.c
> > index 74d5dbd..0d3de67 100644
> > --- a/libc/stdlib/malloc/memalign.c
> > +++ b/libc/stdlib/malloc/memalign.c
> > @@ -77,7 +77,9 @@ memalign (size_t alignment, size_t size)
> >         init_size = addr - tot_addr;
> >       }
> >
> > +      __heap_lock (&__malloc_heap_lock);
> >         __heap_free (heap, base, init_size);
> > +      __heap_unlock (&__malloc_heap_lock);
> >
> >         /* Remember that we've freed the initial part of MEM.  */
> >         base += init_size;
> > @@ -85,9 +87,11 @@ memalign (size_t alignment, size_t size)
> >
> >     /* Return the end part of MEM to the heap, unless it's too small.  */
> >     end_addr = addr + size;
> > -  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr)
> > +  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr) {
> > +    __heap_lock (&__malloc_heap_lock);
> >       __heap_free (heap, (void *)end_addr, tot_end_addr - end_addr);
> > -  else
> > +    __heap_unlock (&__malloc_heap_lock);
> > +  } else
> >       /* We didn't free the end, so include it in the size.  */
> >       end_addr = tot_end_addr;
> >

Patch
diff mbox series

diff --git a/libc/stdlib/malloc/memalign.c b/libc/stdlib/malloc/memalign.c
index 74d5dbd..0d3de67 100644
--- a/libc/stdlib/malloc/memalign.c
+++ b/libc/stdlib/malloc/memalign.c
@@ -77,7 +77,9 @@  memalign (size_t alignment, size_t size)
 	  init_size = addr - tot_addr;
 	}
 
+      __heap_lock (&__malloc_heap_lock);
       __heap_free (heap, base, init_size);
+      __heap_unlock (&__malloc_heap_lock);
 
       /* Remember that we've freed the initial part of MEM.  */
       base += init_size;
@@ -85,9 +87,11 @@  memalign (size_t alignment, size_t size)
 
   /* Return the end part of MEM to the heap, unless it's too small.  */
   end_addr = addr + size;
-  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr)
+  if (end_addr + MALLOC_REALLOC_MIN_FREE_SIZE < tot_end_addr) {
+    __heap_lock (&__malloc_heap_lock);
     __heap_free (heap, (void *)end_addr, tot_end_addr - end_addr);
-  else
+    __heap_unlock (&__malloc_heap_lock);
+  } else
     /* We didn't free the end, so include it in the size.  */
     end_addr = tot_end_addr;