Skip to content

Commit 6c7ac61

Browse files
committed
Packer: store the parent buffer mapped strings on the stack
Since the parent buffer isn't marked while we're executing the extension proc, we need to ensure all the chunks `mapped_strings` won't be GCed nor moved. To do that, we copy all of them onto the stack. It's a bit disgusting, but until we refactor the buffer implementation to support recursive extensions better, it's the simplest solution.
1 parent e48df01 commit 6c7ac61

2 files changed

Lines changed: 39 additions & 0 deletions

File tree

ChangeLog

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
* Fix a potential GC bug when packing data using recursive extensions and buffers containing over 512KkiB of data (See #341).
12
* Fix a regression where feeding an empty string to an Unpacker would be considered like the end of the buffer.
23

34
2023-05-19 1.7.1:

ext/msgpack/packer.c

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,40 @@ bool msgpack_packer_try_write_with_ext_type_lookup(msgpack_packer_t* pk, VALUE v
114114
}
115115

116116
if(ext_flags & MSGPACK_EXT_RECURSIVE) {
117+
// HACK: While we call the proc, the current pk->buffer won't be reachable
118+
// as it will be stored on the stack.
119+
// To ensure all the `mapped_string` reference in that buffer are properly
120+
// marked and pined, we copy them all on the stack.
121+
VALUE* mapped_strings = NULL;
122+
size_t mapped_strings_count = 0;
123+
msgpack_buffer_chunk_t* c = pk->buffer.head;
124+
while (c != &pk->buffer.tail) {
125+
if (c->mapped_string != NO_MAPPED_STRING) {
126+
mapped_strings_count++;
127+
}
128+
c = c->next;
129+
}
130+
if (c->mapped_string != NO_MAPPED_STRING) {
131+
mapped_strings_count++;
132+
}
133+
134+
if (mapped_strings_count > 0) {
135+
mapped_strings = ALLOCA_N(VALUE, mapped_strings_count);
136+
mapped_strings_count = 0;
137+
c = pk->buffer.head;
138+
while (c != &pk->buffer.tail) {
139+
if (c->mapped_string != NO_MAPPED_STRING) {
140+
mapped_strings[mapped_strings_count] = c->mapped_string;
141+
mapped_strings_count++;
142+
}
143+
c = c->next;
144+
}
145+
if (c->mapped_string != NO_MAPPED_STRING) {
146+
mapped_strings[mapped_strings_count] = c->mapped_string;
147+
mapped_strings_count++;
148+
}
149+
}
150+
117151
msgpack_buffer_t parent_buffer = pk->buffer;
118152
msgpack_buffer_init(PACKER_BUFFER_(pk));
119153

@@ -132,6 +166,10 @@ bool msgpack_packer_try_write_with_ext_type_lookup(msgpack_packer_t* pk, VALUE v
132166
pk->buffer = parent_buffer;
133167
msgpack_packer_write_ext(pk, ext_type, payload);
134168
}
169+
170+
if (mapped_strings_count > 0) {
171+
RB_GC_GUARD(mapped_strings[0]);
172+
}
135173
} else {
136174
VALUE payload = rb_proc_call_with_block(proc, 1, &v, Qnil);
137175
StringValue(payload);

0 commit comments

Comments
 (0)