<div dir="ltr">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:<div><br></div><div><div><font face="monospace, monospace"> if (node == NULL || (uintptr_t)extent_node_addr_get(curnode) <</font></div><div><font face="monospace, monospace"> (uintptr_t)extent_node_addr_get(node))</font></div></div><div><div><font face="monospace, monospace"> node = curnode;</font></div></div><div><br></div><div>To be something like:</div><div><br></div><div><div><font face="monospace, monospace"> if (size <= extend_node_size_get(curnode) && (node == NULL ||</font></div><div><font face="monospace, monospace"> (uintptr_t)extent_node_addr_get(curnode) <</font></div><div><font face="monospace, monospace"> (uintptr_t)extent_node_addr_get(node)))</font></div><div><font face="monospace, monospace"> node = curnode;</font></div></div><div><br></div><div>There might be a better way to do this given knowledge of how chunks are stored, but I'm not sure.</div><div><br></div><div>Christopher</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 2:56 PM, Daniel Micay <span dir="ltr"><<a href="mailto:danielmicay@gmail.com" target="_blank">danielmicay@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 08/06/15 05:15 PM, Christopher Ferris wrote:<br>
> Recently, it appears that there was a bug introduced in chunk<br>
> allocation. The bug is exposed by this small snippet of code:<br>
><br>
> void* mem = malloc(128*1024*1024);<br>
> printf("mem address %p\n", mem);<br>
> free(mem);<br>
> void* large_alloc = malloc(0x80000081UL);<br>
> printf("large mem %p\n", large_alloc);<br>
> free(large_alloc);<br>
><br>
> It looks like the bug is in the chunk_recycle code, in this piece of code:<br>
><br>
> if (new_addr != NULL) {<br>
> extent_node_t key;<br>
> extent_node_init(&key, arena, new_addr, alloc_size, false);<br>
> node = extent_tree_ad_search(chunks_ad, &key);<br>
> } else {<br>
> node = chunk_first_fit(arena, chunks_szad, chunks_ad,<br>
> alloc_size);<br>
> }<br>
> if (node == NULL || (new_addr != NULL &&<br>
> extent_node_size_get(node) <<br>
> size)) {<br>
> malloc_mutex_unlock(&arena->chunks_mtx);<br>
> return (NULL);<br>
> }<br>
><br>
> The problem is that new_addr == NULL, so the size check is not<br>
> performed. In my testing, removing the new_addr != NULL check fixes the<br>
> problem, but I don't know if that's the correct change.<br>
><br>
> The first allocation after the free shows the problem, if you try and<br>
> use the whole memory allocation it might segfault, or let you scribble<br>
> all over someone else's memory.<br>
<br>
</div></div>It only checks when new_addr isn't NULL because it does an address-based<br>
map lookup without taking into account the size. The real bug in this<br>
case would be inside chunk_first_fit(...) because it should only be<br>
returning nodes with a size greater than or equal to the requested size<br>
(or NULL).<br>
<br>
In the past, it also only did that size check for new_addr != NULL but<br>
it used first-best-fit instead of first-fit so it only had to do a<br>
single map lookup ordered by (size, addr) instead of calling that more<br>
complex new chunk_first_fit code.<br>
<br>
<br>_______________________________________________<br>
jemalloc-discuss mailing list<br>
<a href="mailto:jemalloc-discuss@canonware.com">jemalloc-discuss@canonware.com</a><br>
<a href="http://www.canonware.com/mailman/listinfo/jemalloc-discuss" rel="noreferrer" target="_blank">http://www.canonware.com/mailman/listinfo/jemalloc-discuss</a><br>
<br></blockquote></div><br></div>