Skip to content
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

_msgpack_buffer_add_new_chunk zero-out the newly allocated tail #343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

casperisfine
Copy link

Fix: #342
Ref: #341

Reseting the memory in _msgpack_buffer_alloc_new_chunk was pointless because the previous tail is immediately copied into it, and it's the tail that is then used by the caller. So it's the tail we should have zeroed-out.

I think what is happening is in _msgpack_buffer_expand:

    if(b->tail.mapped_string != NO_MAPPED_STRING || capacity <= MSGPACK_RMEM_PAGE_SIZE) {
        /* allocate new chunk */
        _msgpack_buffer_add_new_chunk(b);

        char* mem = _msgpack_buffer_chunk_malloc(b, &b->tail, length, &capacity);

        char* last = mem;
        if(data != NULL) {
            memcpy(mem, data, length);
            last += length;
        }

        /* rebuild tail chunk */
        b->tail.first = mem;
        b->tail.last = last;
        b->tail.mapped_string = NO_MAPPED_STRING;
        b->tail_buffer_end = mem + capacity;

_msgpack_buffer_add_new_chunk potentially leave b->tail.mapped_string as a garbage reference, then _msgpack_buffer_chunk_malloc may trigger a GC.

b->tail.mapped_string is only reset later.

I'm still trying to craft a repro for such error case.

Fix: msgpack#342

Reseting the memory in _msgpack_buffer_alloc_new_chunk was pointless
because the previous `tail` is immediately copied into it, and it's
the `tail` that is then used by the caller. So it's the `tail` we
should zero-out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants