malloc_set_state and heap content
Commit Message
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
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
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
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
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
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
@@ -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):
@@ -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;