Fix rmem page released while a later chunk still points into it

[Watson1978]

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:

/* update rmem owner */
c->mem = *b->rmem_owner;
*b->rmem_owner = NULL;
b->rmem_owner = &c->mem;

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.

require 'msgpack'

threshold = 1024
long = 'L' * (threshold * 2)

b1 = MessagePack::Buffer.new(nil, write_reference_threshold: threshold)
b1.write('a' * 100)  # first rmem page
b1.write(long)       # written by reference
b1.write('b' * 10)   # second rmem page, left mostly unused
b1.write(long)       # reclaims the unused part of that page
b1.write('c' * 50)   # carved out of the reclaimed part
b1.read(100 + long.bytesize + 10 + long.bytesize)

6.times { |i| b = MessagePack::Buffer.new; b.write(('A'.ord + i).chr * 4000) }

p b1.read_all

Before this patch:

"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"

After:

"cccccccccccccccccccccccccccccccccccccccccccccccccc"

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.

• rake spec: 458 examples, 0 failures
• rake spec:valgrind: 458 examples, 0 failures, no memcheck errors

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

[byroot]

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.

[Watson1978]

Thanks for the quick review.

I share the feeling. Both issues this patch fixes are in the arena's ownership
bookkeeping (rmem_owner / rmem_last), not in the buffer logic itself, and
s_rmem being a lock-free process global is a hard blocker for Ractor support.

Removing it sounds reasonable to me.