Bug in chunk allocation

[Christopher Ferris]

So it appears a better fix is to modify chunk_first_fit to return NULL in
this particular case. I'm not as familiar with exactly how the chunks are
stored, so I don't know if I can propose the best fix. For example, it's
possible to modify this check in chunk_first_fit:

               if (node == NULL || (uintptr_t)extent_node_addr_get(curnode)
<
                    (uintptr_t)extent_node_addr_get(node))
                        node = curnode;

To be something like:

                if (size <= extend_node_size_get(curnode) && (node == NULL
||
                    (uintptr_t)extent_node_addr_get(curnode) <
                    (uintptr_t)extent_node_addr_get(node)))
                        node = curnode;

There might be a better way to do this given knowledge of how chunks are
stored, but I'm not sure.

Christopher

On Mon, Jun 8, 2015 at 2:56 PM, Daniel Micay <danielmicay at gmail.com> wrote:

> On 08/06/15 05:15 PM, Christopher Ferris wrote:
> > Recently, it appears that there was a bug introduced in chunk
> > allocation. The bug is exposed by this small snippet of code:
> >
> >   void* mem = malloc(128*1024*1024);
> >   printf("mem address %p\n", mem);
> >   free(mem);
> >   void* large_alloc = malloc(0x80000081UL);
> >   printf("large mem %p\n", large_alloc);
> >   free(large_alloc);
> >
> > It looks like the bug is in the chunk_recycle code, in this piece of
> code:
> >
> >         if (new_addr != NULL) {
> >                 extent_node_t key;
> >                 extent_node_init(&key, arena, new_addr, alloc_size,
> false);
> >                 node = extent_tree_ad_search(chunks_ad, &key);
> >         } else {
> >                 node = chunk_first_fit(arena, chunks_szad, chunks_ad,
> >                     alloc_size);
> >         }
> >         if (node == NULL || (new_addr != NULL &&
> > extent_node_size_get(node) <
> >             size)) {
> >                 malloc_mutex_unlock(&arena->chunks_mtx);
> >                 return (NULL);
> >         }
> >
> > The problem is that new_addr == NULL, so the size check is not
> > performed. In my testing, removing the new_addr != NULL check fixes the
> > problem, but I don't know if that's the correct change.
> >
> > The first allocation after the free shows the problem, if you try and
> > use the whole memory allocation it might segfault, or let you scribble
> > all over someone else's memory.
>
> It only checks when new_addr isn't NULL because it does an address-based
> map lookup without taking into account the size. The real bug in this
> case would be inside chunk_first_fit(...) because it should only be
> returning nodes with a size greater than or equal to the requested size
> (or NULL).
>
> In the past, it also only did that size check for new_addr != NULL but
> it used first-best-fit instead of first-fit so it only had to do a
> single map lookup ordered by (size, addr) instead of calling that more
> complex new chunk_first_fit code.
>
>
> _______________________________________________
> jemalloc-discuss mailing list
> jemalloc-discuss at canonware.com
> http://www.canonware.com/mailman/listinfo/jemalloc-discuss
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://jemalloc.net/mailman/jemalloc-discuss/attachments/20150615/03a27b75/attachment.html>