diff mbox

malloc_set_state and heap content

Message ID 20160706193547.GD8550@var.home
State New
Headers show

Commit Message

Samuel Thibault July 6, 2016, 7:35 p.m. UTC 
Hello,

In 4cf6c72fd2a482e7499c29162349810029632c3f ('malloc: Rewrite dumped
heap for compatibility in __malloc_set_state'), __malloc_set_state was
reimplemented, using the following look to detect the first chunk of the
heap:

  /* Find the chunk with the lowest address with the heap.  */
  mchunkptr chunk = NULL;
  {
    size_t *candidate = (size_t *) ms->sbrk_base;
    size_t *end = (size_t *) (ms->sbrk_base + ms->sbrked_mem_bytes);
    while (candidate < end)
      if (*candidate != 0)
       {
         chunk = mem2chunk ((void *) (candidate + 1));
         break;
       }
      else
       ++candidate;

That assumes that the beginning of the heap is zeroed.

It happens that in malloc/malloc.c one can read:

  /*
     Skip over some bytes to arrive at an aligned position.
     We don't need to specially mark these wasted front bytes.
     They will never be accessed anyway because
     prev_inuse of av->top (and any chunk created from its start)
     is always true after initialization.
   */

On Linux the space happens to be zero by luck, but with other kernels
that may not be true (it is not with the Hurd).  Also, only the 'size'
field of the first chunk is initialized by

set_head (av->top, (snd_brk - aligned_brk + correction) | PREV_INUSE);

So I'd say we need the attached patch, don't we?

Samuel

Comments

Florian Weimer July 6, 2016, 8:36 p.m. UTC | #1 
On 07/06/2016 09:35 PM, Samuel Thibault wrote:
> Hello,
>
> In 4cf6c72fd2a482e7499c29162349810029632c3f ('malloc: Rewrite dumped
> heap for compatibility in __malloc_set_state'), __malloc_set_state was
> reimplemented, using the following look to detect the first chunk of the
> heap:
>
>   /* Find the chunk with the lowest address with the heap.  */
>   mchunkptr chunk = NULL;
>   {
>     size_t *candidate = (size_t *) ms->sbrk_base;
>     size_t *end = (size_t *) (ms->sbrk_base + ms->sbrked_mem_bytes);
>     while (candidate < end)
>       if (*candidate != 0)
>        {
>          chunk = mem2chunk ((void *) (candidate + 1));
>          break;
>        }
>       else
>        ++candidate;
>
> That assumes that the beginning of the heap is zeroed.

Yes.  There is no in-tree glibc port which sets MORECORE_CLEARS to zero, 
so malloc already assumes that the memory is cleared.

> On Linux the space happens to be zero by luck, but with other kernels
> that may not be true (it is not with the Hurd).

How gets Hurd away with that without introducing a security 
vulnerability?  Why is MORECORE_CLEARS not defined as 0?

> So I'd say we need the attached patch, don't we?

The patch does not address the issue because it does not alter the heap 
copy in existing Emacs binaries.  It would only become effective after 
recompiling Emacs.  Such recompiled Emacs binaries will no longer use 
the heap dumping mechanism.

Florian
Samuel Thibault July 6, 2016, 8:38 p.m. UTC | #2 
Florian Weimer, on Wed 06 Jul 2016 22:36:05 +0200, wrote:
> On 07/06/2016 09:35 PM, Samuel Thibault wrote:
> >On Linux the space happens to be zero by luck, but with other kernels
> >that may not be true (it is not with the Hurd).
> 
> How gets Hurd away with that without introducing a security vulnerability?

What remains on the heap is initialization stuff, not remainders from
pages allocated by the kernel.

> >So I'd say we need the attached patch, don't we?
> 
> The patch does not address the issue because it does not alter the heap copy
> in existing Emacs binaries.  It would only become effective after
> recompiling Emacs.  Such recompiled Emacs binaries will no longer use the
> heap dumping mechanism.

Well, glibc is not only about emacs, is it? :)

Samuel
Florian Weimer July 6, 2016, 9:09 p.m. UTC | #3 
On 07/06/2016 10:38 PM, Samuel Thibault wrote:
> Florian Weimer, on Wed 06 Jul 2016 22:36:05 +0200, wrote:
>> On 07/06/2016 09:35 PM, Samuel Thibault wrote:
>>> On Linux the space happens to be zero by luck, but with other kernels
>>> that may not be true (it is not with the Hurd).
>>
>> How gets Hurd away with that without introducing a security vulnerability?
>
> What remains on the heap is initialization stuff, not remainders from
> pages allocated by the kernel.

I still don't see how this is correct.  Maybe the Hurd startup code 
mallocs so much that it consumes all that data before the application 
can call calloc.  Otherwise, an early calloc call would return non-zero 
memory.

>>> So I'd say we need the attached patch, don't we?
>>
>> The patch does not address the issue because it does not alter the heap copy
>> in existing Emacs binaries.  It would only become effective after
>> recompiling Emacs.  Such recompiled Emacs binaries will no longer use the
>> heap dumping mechanism.
>
> Well, glibc is not only about emacs, is it? :)

Could you be more specific, please?  Are there Hurd-specific 
applications which use malloc_set_state?

Thanks,
Florian
Samuel Thibault July 6, 2016, 9:11 p.m. UTC | #4 
Florian Weimer, on Wed 06 Jul 2016 23:09:20 +0200, wrote:
> On 07/06/2016 10:38 PM, Samuel Thibault wrote:
> >Florian Weimer, on Wed 06 Jul 2016 22:36:05 +0200, wrote:
> >>On 07/06/2016 09:35 PM, Samuel Thibault wrote:
> >>>So I'd say we need the attached patch, don't we?
> >>
> >>The patch does not address the issue because it does not alter the heap copy
> >>in existing Emacs binaries.  It would only become effective after
> >>recompiling Emacs.  Such recompiled Emacs binaries will no longer use the
> >>heap dumping mechanism.
> >
> >Well, glibc is not only about emacs, is it? :)
> 
> Could you be more specific, please?  Are there Hurd-specific applications
> which use malloc_set_state?

I mean that there can be other applications making use of
malloc_set_state

https://codesearch.debian.net/search?q=malloc_set_state

suggests a few.

So it's worth fixing it for those anyway.

Samuel
Florian Weimer July 6, 2016, 9:20 p.m. UTC | #5 
On 07/06/2016 11:11 PM, Samuel Thibault wrote:
> Florian Weimer, on Wed 06 Jul 2016 23:09:20 +0200, wrote:
>> On 07/06/2016 10:38 PM, Samuel Thibault wrote:
>>> Florian Weimer, on Wed 06 Jul 2016 22:36:05 +0200, wrote:
>>>> On 07/06/2016 09:35 PM, Samuel Thibault wrote:
>>>>> So I'd say we need the attached patch, don't we?
>>>>
>>>> The patch does not address the issue because it does not alter the heap copy
>>>> in existing Emacs binaries.  It would only become effective after
>>>> recompiling Emacs.  Such recompiled Emacs binaries will no longer use the
>>>> heap dumping mechanism.
>>>
>>> Well, glibc is not only about emacs, is it? :)
>>
>> Could you be more specific, please?  Are there Hurd-specific applications
>> which use malloc_set_state?
>
> I mean that there can be other applications making use of
> malloc_set_state
>
> https://codesearch.debian.net/search?q=malloc_set_state
>
> suggests a few.

Which ones?

As far I can tell, those are either automatically generated wrappers 
which are unlikely to work at all (because they are for high-level 
languages which call malloc before the application code has a chance to 
call malloc_set_state), or copies of dlmalloc or ptmalloc (so they are 
*implementations* of malloc_set_state).

In any case, your proposed fix is incorrect because it does not address 
backwards compatibility issues with slightly corrupted dumped heaps in 
non-Linux binaries, and new binaries won't be able to use 
malloc_set_state reliably (even with the current state of master, and we 
are going to turn malloc_set_state into a compat symbol before the release).

Florian
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 690012c..343accd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@ 
+2016-07-06  Samuel Thibault  <samuel.thibault@ens-lyon.org>
+
+	* malloc/malloc.c (sysmalloc): Zero memory between brk and the heap
+	top, and set prev_size of the first chunk to zero, so malloc_set_state
+	can properly find the first chunk.
+
 2016-07-06  Stefan Liebler  <stli@linux.vnet.ibm.com>
 
 	* sysdeps/s390/linkmap.h (struct link_map_machine):
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1f5f166..beb97e9 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2600,13 +2600,12 @@  sysmalloc (INTERNAL_SIZE_T nb, mstate av)
                     {
                       /*
                          Skip over some bytes to arrive at an aligned position.
-                         We don't need to specially mark these wasted front bytes.
-                         They will never be accessed anyway because
-                         prev_inuse of av->top (and any chunk created from its start)
-                         is always true after initialization.
+                         We zero them for malloc_set_state to properly find the
+                         first chunk.
                        */
 
                       correction = MALLOC_ALIGNMENT - front_misalign;
+                      memset (brk, 0, correction);
                       aligned_brk += correction;
                     }
 
@@ -2661,13 +2660,13 @@  sysmalloc (INTERNAL_SIZE_T nb, mstate av)
                         {
                           /*
                              Skip over some bytes to arrive at an aligned position.
-                             We don't need to specially mark these wasted front bytes.
-                             They will never be accessed anyway because
-                             prev_inuse of av->top (and any chunk created from its start)
-                             is always true after initialization.
+                             We zero them for malloc_set_state to properly find
+                             the first chunk.
                            */
 
-                          aligned_brk += MALLOC_ALIGNMENT - front_misalign;
+                          correction = MALLOC_ALIGNMENT - front_misalign;
+                          memset (brk, 0, correction);
+                          aligned_brk += correction;
                         }
                     }
 
@@ -2682,6 +2681,7 @@  sysmalloc (INTERNAL_SIZE_T nb, mstate av)
               if (snd_brk != (char *) (MORECORE_FAILURE))
                 {
                   av->top = (mchunkptr) aligned_brk;
+                  av->top->prev_size = 0;
                   set_head (av->top, (snd_brk - aligned_brk + correction) | PREV_INUSE);
                   av->system_mem += correction;