New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clear_chunk_list accesses freed memory; could crash or corrupt heap #32
Comments
Hi snej, Thanks for your report. I think the current code accesses freed memory too. But I have a different idea to fix this problem. First, init_chunk_list is called. Then the zone's memory is like the following diagram.
After msgpack_zone_malloc_expand is called, Then the zone's memory is like the following diagram.
When clear_chunk_list calls, let's see step by step.
c is point to cl->head as follows:
c->next is not NULL, then free(c) and c points to the next chunk.
The next iteration, c->next is NULL, then break the loop. I guess that the intention of clear_chunk_list() is making the same chunk state as just after init_chunk_list() called. So I think that cl->head should be updated with c before accessing cl->head->next
As a result of this modification, the chunk state after calling clear_chunk_list() is the following:
I will ask the clear_chunk_list() intention to the original author. If my understanding is correct, I would write and apply patch. |
I think you're right about the expected behavior, and your suggestion sounds good. Thanks! |
nobu_k, https://github.com/nobu-k , pointed out my mistake.
should be
I wrote "cl->head should be updated with c before accessing cl->head->next". |
merged. |
The Clang static analyzer points out that the function
clear_chunk_list
accesses memory after it's been freed, in the line:The malloc block pointed to by
cl->head
has already been freed up above in the lineThe consequences of this are pretty dire. Writing into a free block is likely to corrupt heap structures (depending on the malloc implementation). It could crash immediately if the VM page was freed. Or if the block has already been handed to a malloc call on another thread, it would corrupt another program heap block.
It looks as though the fix is to change the offending line to
although I'm not exactly sure what this function is supposed to do. If the zone is supposed to remain usable, restored to the state it was initially in after
init_chunk_list
, then this isn't the right fix. Instead it should probably be freeing only the chunks that come after the first one, which would mean modifying the while loop slightly.The text was updated successfully, but these errors were encountered: