Skip to content

Commit ce44d0e

Browse files
Watson1978claude
andcommitted
Fix rmem page released while a later chunk still points into it
_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>
1 parent 8ab8a5a commit ce44d0e

2 files changed

Lines changed: 68 additions & 0 deletions

File tree

ext/msgpack/buffer.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@ void msgpack_buffer_mark(void *ptr)
127127

128128
bool _msgpack_buffer_shift_chunk(msgpack_buffer_t* b)
129129
{
130+
if(b->rmem_owner == &b->head->mem) {
131+
/* the chunk that owns the rmem page is going away and takes the page
132+
* with it. don't carve the remaining space out of a page that is
133+
* already back in the pool. */
134+
b->rmem_end = NULL;
135+
b->rmem_last = NULL;
136+
b->rmem_owner = NULL;
137+
}
138+
130139
_msgpack_buffer_chunk_destroy(b->head);
131140

132141
if(b->head == &b->tail) {
@@ -264,6 +273,18 @@ static inline msgpack_buffer_chunk_t* _msgpack_buffer_alloc_new_chunk(msgpack_bu
264273
return chunk;
265274
}
266275

276+
/* b->tail is copied into nc and then rebuilt, so the rmem page pointer moves
277+
* to nc. the owner has to follow it: if it kept pointing at b->tail.mem, the
278+
* transfer in _msgpack_buffer_chunk_malloc would degenerate into a
279+
* self-assignment and the page would be released by nc while the rebuilt tail
280+
* is still reading from it. */
281+
static inline void _msgpack_buffer_transfer_rmem_owner(msgpack_buffer_t* b, msgpack_buffer_chunk_t* nc)
282+
{
283+
if(b->rmem_owner == &b->tail.mem) {
284+
b->rmem_owner = &nc->mem;
285+
}
286+
}
287+
267288
static inline void _msgpack_buffer_add_new_chunk(msgpack_buffer_t* b)
268289
{
269290
if(b->head == &b->tail) {
@@ -275,6 +296,7 @@ static inline void _msgpack_buffer_add_new_chunk(msgpack_buffer_t* b)
275296
msgpack_buffer_chunk_t* nc = _msgpack_buffer_alloc_new_chunk(b);
276297

277298
*nc = b->tail;
299+
_msgpack_buffer_transfer_rmem_owner(b, nc);
278300
b->head = nc;
279301
nc->next = &b->tail;
280302

@@ -295,6 +317,7 @@ static inline void _msgpack_buffer_add_new_chunk(msgpack_buffer_t* b)
295317

296318
/* rebuild tail */
297319
*nc = b->tail;
320+
_msgpack_buffer_transfer_rmem_owner(b, nc);
298321
before_tail->next = nc;
299322
nc->next = &b->tail;
300323
}

spec/cruby/buffer_spec.rb

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,4 +620,49 @@
620620

621621
expect(b1.read_all).to eq(('C' * 128).b)
622622
end
623+
624+
it "keeps an rmem page alive while a later chunk still points into it" do
625+
threshold = 1024
626+
long = 'L' * (threshold * 2)
627+
628+
b1 = MessagePack::Buffer.new(nil, write_reference_threshold: threshold)
629+
b1.write('a' * 100) # first rmem page
630+
b1.write(long) # written by reference
631+
b1.write('b' * 10) # second rmem page, left mostly unused
632+
b1.write(long) # reclaims the unused part of that page
633+
b1.write('c' * 50) # carved out of the reclaimed part
634+
b1.read(100 + long.bytesize + 10 + long.bytesize)
635+
636+
# the pool must not hand that page to anyone else while b1 still reads from it
637+
others = 6.times.map do |i|
638+
b = MessagePack::Buffer.new
639+
b.write(('A'.ord + i).chr * 4000)
640+
b
641+
end
642+
643+
expect(b1.read_all).to eq(('c' * 50).b)
644+
expect(others.size).to eq(6)
645+
end
646+
647+
it "does not carve a new chunk out of an rmem page it already released" do
648+
threshold = 1024
649+
long = 'L' * (threshold * 2)
650+
651+
b1 = MessagePack::Buffer.new(nil, write_reference_threshold: threshold)
652+
b1.write('a' * 100)
653+
b1.write(long)
654+
b1.write('b' * 10)
655+
b1.write(long) # reclaims the unused part of the page
656+
b1.read(100 + long.bytesize + 10) # releases that very page
657+
b1.write('d' * 50) # must not be carved out of it
658+
659+
others = 6.times.map do |i|
660+
b = MessagePack::Buffer.new
661+
b.write(('a'.ord + i).chr * 4000)
662+
b
663+
end
664+
665+
expect(b1.read_all).to eq((long + ('d' * 50)).b)
666+
expect(others.size).to eq(6)
667+
end
623668
end

0 commit comments

Comments
 (0)