| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
_msgpack_buffer_chunk_malloc hands the current rmem page over to the chunk that carves the reclaimed space out of it, but b->rmem_owner was always an alias of &b->tail.mem: _msgpack_buffer_add_new_chunk copies b->tail into a new chunk without repointing the owner, so the transfer degenerated into a self-assignment that immediately nulled the pointer. The page stayed owned by the older chunk instead, and because chunks are destroyed head first, it was returned to the process global pool while the rebuilt tail was still reading from it. Another buffer allocating a page then received the very same memory: reads returned that buffer's bytes and further writes corrupted it. Let the owner follow the page when the tail is copied, so the newest chunk carved out of a page is the one that releases it. Also drop the carve window in _msgpack_buffer_shift_chunk when the chunk owning the current page is destroyed. rmem_last / rmem_end kept pointing into that page, so a later small write could carve a new chunk out of a page that was already back in the pool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
That arena implementation keep causing issues while I'm still not convinced of it efficiency 😫. I'll review, merge and publish your patch, but given the arena is a problem for Ractors, I'm tempted to just remove it in the near future. |
Sorry, something went wrong.
|
Thanks for the quick review. I share the feeling. Both issues this patch fixes are in the arena's ownership Removing it sounds reasonable to me. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
A MessagePack::Buffer (and therefore Packer) can serve bytes out of an rmem page that has already been returned to the process global pool and handed to a different buffer. Reads return the other buffer's data, and further writes corrupt it. Nothing crashes, so the two buffers silently exchange content.
In a server that serializes per request, that means bytes of one request's payload can surface inside another request's buffer.
Root cause
_msgpack_buffer_chunk_malloc carves a new chunk out of the unused tail of the current rmem page, and transfers ownership of the page to that new chunk so the page is released only when the last chunk using it dies:
The transfer never happened, because b->rmem_owner was always an alias of &b->tail.mem. It is only ever assigned as &c->mem, and c is &b->tail at every call site, so the three lines above degenerate into a self-assignment that is immediately nulled.
The page pointer had in fact moved elsewhere: _msgpack_buffer_add_new_chunk copies the tail into a freshly allocated chunk with *nc = b->tail, which duplicates mem into nc, but leaves b->rmem_owner pointing at the old location. Ownership therefore stayed with the older chunk. Chunks are destroyed head first, so that chunk released the page while the rebuilt tail was still reading from it.
The fix lets the owner follow the page when the tail is copied, so the newest chunk carved out of a page is the one that releases it.
Second, related hole
While confirming the above, a sibling case turned up: _msgpack_buffer_shift_chunk destroys a chunk without invalidating rmem_last / rmem_end. When the chunk being destroyed is the one owning the current page, the page goes back to the pool but the carve window still points into it, so a later small write carves a new chunk out of a page that another buffer may already own. This patch drops the carve window in that case, which costs at most one page's worth of reuse.
The first fix alone does not close this one.
Reproduction
Both cases use only public API. The write_reference_threshold is lowered so the reference-append that leaves an rmem page partly unused can be triggered with short strings; the default 512 KB threshold reaches the same state with larger writes.
Before this patch:
After:
Tests
Two examples were added to spec/cruby/buffer_spec.rb, one per case above. Both fail on the current master build with the other buffer's bytes in place of the expected content, and pass with this patch.
Notes
This is not a use-after-free in the libc sense in the common case. msgpack_rmem_free only flips a bit in the pool's mask, and the backing 128 KB block stays allocated, so the access is to live memory and the failure is silent aliasing rather than a fault. It does become a genuine heap use-after-free in the case where the whole rmem chunk is drained and _msgpack_rmem_chunk_free calls xfree(c->pages) while a buffer chunk still points into that block.
🤖 Generated with Claude Code