clear_chunk_list accesses freed memory; could crash or corrupt heap

[snej]

The Clang static analyzer points out that the function clear_chunk_list accesses memory after it's been freed, in the line:

cl->head->next = NULL;

The malloc block pointed to by cl->head has already been freed up above in the line

free(c);

The 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

cl->head = NULL

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.

[redboltz]

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.

          NULL
            A
            |next
         +--|--+---- - - - ---+
head---> |  +  |              |
         +-----+---- - - - ---+  

After msgpack_zone_malloc_expand is called, Then the zone's memory is like the following diagram.

          NULL
            A
            |next
         +--|--+---- - - - ---+
         |  +  |              |
         +-----+---- - - - ---+  
            A
            |next
         +--|--+---- - - - ---+
head---> |  +  |              |
         +-----+---- - - - ---+  

When clear_chunk_list calls, let's see step by step.

static inline void clear_chunk_list(msgpack_zone_chunk_list* cl, size_t chunk_size)
{
    msgpack_zone_chunk* c = cl->head;
    while(true) {
        msgpack_zone_chunk* n = c->next;
        if(n != NULL) {
            free(c);
            c = n;
        } else {
            break;
        }
    }
    cl->head->next = NULL;
    cl->free = chunk_size;
    cl->ptr  = ((char*)cl->head) + sizeof(msgpack_zone_chunk);
}

c is point to cl->head as follows:

          NULL
            A
            |next
         +--|--+---- - - - ---+
         |  +  |              |
         +-----+---- - - - ---+  
            A
            |next
         +--|--+---- - - - ---+
head---> |  +  |              |<---c
         +-----+---- - - - ---+  

c->next is not NULL, then free(c) and c points to the next chunk.

          NULL
            A
            |next
         +--|--+---- - - - ---+
         |  +  |              |<---c
         +-----+---- - - - ---+  
            A
            |next
         +--|--+---- - - - ---+
head---> |  +  |  FREED       |
         +-----+---- - - - ---+  

The next iteration, c->next is NULL, then break the loop.
So far, the head points to the FREED memory. Then writes head->next, FREED memory.

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

static inline void clear_chunk_list(msgpack_zone_chunk_list* cl, size_t chunk_size)
{
    msgpack_zone_chunk* c = cl->head;
    while(true) {
        msgpack_zone_chunk* n = c->next;
        if(n != NULL) {
            free(c);
            c = n;
        } else {
            cl->next = c; // ***Add this line***
            break;
        }
    }
    cl->head->next = NULL;
    cl->free = chunk_size;
    cl->ptr  = ((char*)cl->head) + sizeof(msgpack_zone_chunk);
}

As a result of this modification, the chunk state after calling clear_chunk_list() is the following:

          NULL
            A
            |next
         +--|--+---- - - - ---+
head---> |  +  |              |<---c
         +-----+---- - - - ---+  
            A
            |next
         +--|--+---- - - - ---+
         |  +  |  FREED       |
         +-----+---- - - - ---+  

I will ask the clear_chunk_list() intention to the original author. If my understanding is correct, I would write and apply patch.

[snej]

I think you're right about the expected behavior, and your suggestion sounds good. Thanks!

[redboltz]

nobu_k, https://github.com/nobu-k , pointed out my mistake.

            cl->next = c; // ***Add this line***

should be

            cl->head = c; // ***Add this line***

I wrote "cl->head should be updated with c before accessing cl->head->next".
Thanks nobu_k.

[nobu-k]

Thank you for the detailed report, @snej! I think @redboltz's solution looks good, too.

[redboltz]

merged.