diff --git a/ext/msgpack/buffer.h b/ext/msgpack/buffer.h index 1c8efe54..de8d8b88 100644 --- a/ext/msgpack/buffer.h +++ b/ext/msgpack/buffer.h @@ -237,14 +237,27 @@ void _msgpack_buffer_append_long_string(msgpack_buffer_t* b, VALUE string); static inline size_t msgpack_buffer_append_string(msgpack_buffer_t* b, VALUE string) { - size_t length; - char *ptr; - RSTRING_GETMEM(string, ptr, length); + size_t length = RSTRING_LEN(string); if(length > b->write_reference_threshold) { _msgpack_buffer_append_long_string(b, string); - } else { - msgpack_buffer_append(b, ptr, length); + } else if(length > 0) { + /* Making room can run io.write or trigger GC, either of which may free + * or shrink the string, so reserve the space first and only then read + * its pointer. */ + msgpack_buffer_ensure_writable(b, length); + char *ptr; + size_t current; + RSTRING_GETMEM(string, ptr, current); + if(current > length) { + current = length; + } + memcpy(b->tail.last, ptr, current); + if(current < length) { + memset(b->tail.last + current, 0, length - current); + } + b->tail.last += length; + RB_GC_GUARD(string); } return length; diff --git a/spec/packer_spec.rb b/spec/packer_spec.rb index 3968622f..09268acc 100644 --- a/spec/packer_spec.rb +++ b/spec/packer_spec.rb @@ -616,4 +616,32 @@ def to_msgpack_ext packer.write(top_hash) packer.full_pack end + + it 'does not copy freed memory when a string is freed during io.write' do + skip if IS_JRUBY + + size = 100_000 # larger than the io buffer, smaller than the write reference threshold + src = "A".b * size + captured = "".b + flipped = false + io = Object.new + io.define_singleton_method(:write) do |data| + unless flipped + flipped = true + src.replace("".b) # free the buffer the packer still points at + GC.start + Array.new(20) { "Z".b * size } # reuse the freed region + end + captured << data.b + data.bytesize + end + + packer = MessagePack::Packer.new(io) + packer.write(src) + packer.full_pack + + body = MessagePack.unpack(captured) + expect(body.bytesize).to eq(size) + expect(body.b.count("Z".b)).to eq(0) # must not surface the reused heap region + end end