From 8cada9f85e8b069a40fab8828a833f714b5583b8 Mon Sep 17 00:00:00 2001 From: Shizuo Fujita Date: Wed, 30 Sep 2026 11:12:52 +0900 Subject: [PATCH] Fix use-after-free when a recursive extension proc rescues, skips or resets A recursive extension proc that rescued an error from the unpacker, called Unpacker#skip or called Unpacker#reset dropped stack.depth below the enclosing containers. Those containers were no longer GC-marked while the proc ran, so they could be collected before the parser wrote to them. Track the innermost recursive barrier as a stack floor. Error handling and reset now return depth to the floor instead of 0, and skip stops at the barrier like read does. Co-Authored-By: Claude Opus 5.5 --- ext/msgpack/unpacker.c | 16 +++++++---- ext/msgpack/unpacker.h | 6 ++++ ext/msgpack/unpacker_class.c | 2 +- spec/factory_spec.rb | 55 ++++++++++++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 11 deletions(-) diff --git a/ext/msgpack/unpacker.c b/ext/msgpack/unpacker.c index 8b36307d..baae0dd5 100644 --- a/ext/msgpack/unpacker.c +++ b/ext/msgpack/unpacker.c @@ -110,6 +110,7 @@ static inline void _msgpack_unpacker_free_stack(msgpack_unpacker_stack_t* stack) } stack->data = NULL; stack->depth = 0; + stack->floor = 0; } } @@ -169,7 +170,7 @@ void _msgpack_unpacker_reset(msgpack_unpacker_t* uk) uk->head_byte = HEAD_BYTE_REQUIRED; /*memset(uk->stack, 0, sizeof(msgpack_unpacker_t) * uk->stack.depth);*/ - uk->stack.depth = 0; + msgpack_unpacker_stack_rewind(uk); uk->last_object = Qnil; uk->reading_raw = Qnil; uk->reading_raw_remaining = 0; @@ -387,13 +388,14 @@ static inline int read_raw_body_begin(msgpack_unpacker_t* uk, int raw_type) return PRIMITIVE_STACK_TOO_DEEP; } size_t barrier_depth = uk->stack.depth; + size_t saved_floor = uk->stack.floor; int raised; + + uk->stack.floor = barrier_depth; obj = protected_proc_call(proc, 1, &uk->self, &raised); + uk->stack.floor = saved_floor; - /* The user proc can drive the unpacker itself (Unpacker#read, #skip, - * or a rescued error) and leave stack.depth anywhere, including 0. - * Restore it to just below the barrier we pushed instead of an - * unconditional decrement, which would underflow to SIZE_MAX. */ + /* Not --depth: the proc may return with entries left above the barrier. */ uk->stack.depth = barrier_depth - 1; if (raised) { @@ -892,6 +894,10 @@ int msgpack_unpacker_skip(msgpack_unpacker_t* uk, size_t target_stack_depth) container_completed: { msgpack_unpacker_stack_entry_t* top = _msgpack_unpacker_stack_entry_top(uk); + if(top->type == STACK_TYPE_RECURSIVE) { + STACK_FREE(uk); + return PRIMITIVE_OBJECT_COMPLETE; + } /* this section optimized out */ // TODO object_complete still creates objects which should be optimized out diff --git a/ext/msgpack/unpacker.h b/ext/msgpack/unpacker.h index d2314b85..ef4cb71c 100644 --- a/ext/msgpack/unpacker.h +++ b/ext/msgpack/unpacker.h @@ -43,6 +43,7 @@ typedef struct { struct msgpack_unpacker_stack_t { size_t depth; + size_t floor; size_t capacity; msgpack_unpacker_stack_entry_t *data; }; @@ -109,6 +110,11 @@ static inline void msgpack_unpacker_set_allow_unknown_ext(msgpack_unpacker_t* uk uk->allow_unknown_ext = enable; } +static inline void msgpack_unpacker_stack_rewind(msgpack_unpacker_t* uk) +{ + uk->stack.depth = uk->stack.floor; +} + /* error codes */ #define PRIMITIVE_CONTAINER_START 1 diff --git a/ext/msgpack/unpacker_class.c b/ext/msgpack/unpacker_class.c index c4f84699..53c4764f 100644 --- a/ext/msgpack/unpacker_class.c +++ b/ext/msgpack/unpacker_class.c @@ -165,7 +165,7 @@ static VALUE Unpacker_allow_unknown_ext_p(VALUE self) NORETURN(static void raise_unpacker_error(msgpack_unpacker_t *uk, int r)) { - uk->stack.depth = 0; + msgpack_unpacker_stack_rewind(uk); switch(r) { case PRIMITIVE_EOF: rb_raise(rb_eEOFError, "end of buffer reached"); diff --git a/spec/factory_spec.rb b/spec/factory_spec.rb index 2abedab3..b135f383 100644 --- a/spec/factory_spec.rb +++ b/spec/factory_spec.rb @@ -707,11 +707,10 @@ class << Symbol end it 'does not corrupt the stack when a recursive unpacker leaves the depth at zero' do - # A recursive proc that rescues an inner read error, or that calls #skip, - # can drive stack.depth down to 0 before read_raw_body_begin pops its - # barrier. The unconditional pop then underflowed depth to SIZE_MAX and - # read/wrote out-of-bounds stack entries (SIGSEGV). The payloads leave no - # trailing bytes, so a fixed unpacker returns without raising. + # A recursive proc that rescued an inner read error, or that called #skip, + # used to make read_raw_body_begin underflow stack.depth to SIZE_MAX when + # it popped its barrier (SIGSEGV). The payloads leave no trailing bytes, + # so a fixed unpacker returns without raising. skip if IS_JRUBY rescuing = MessagePack::Factory.new @@ -747,6 +746,52 @@ class << Symbol expect(rescuing.unpack(MessagePack.pack([1, 2, 3]))).to eq([1, 2, 3]) expect(skipping.unpack(MessagePack.pack("ok"))).to eq("ok") end + + it 'keeps outer containers GC-marked while a recursive unpacker rescues, skips or resets' do + skip if IS_JRUBY + + factory = MessagePack::Factory.new + factory.register_type(0x01, Class.new, + packer: ->(_obj, packer) { packer.write(nil) }, + unpacker: ->(u) { + begin + u.read + rescue MessagePack::MalformedFormatError + end + [:rescued] + }, + recursive: true, + ) + factory.register_type(0x02, Class.new, + packer: ->(_obj, packer) { packer.write(nil) }, + unpacker: ->(u) { u.skip; [:skipped] }, + recursive: true, + ) + factory.register_type(0x03, Class.new, + packer: ->(_obj, packer) { packer.write(nil) }, + unpacker: ->(u) { u.reset; [:reset] }, + recursive: true, + ) + + payloads = ["\x92\xd4\x01\xc1\x2b".b, "\x92\xd4\x02\x2a\x2b".b, "\x91\xd4\x03".b] + results = [] + begin + GC.stress = true + 10.times do + payloads.each do |bytes| + results << begin + factory.unpack(bytes) + rescue => e + e + end + end + end + ensure + GC.stress = false + end + + expect(results).to eq([[[:rescued], 43], [[:skipped], 43], [[:reset]]] * 10) + end end describe 'memsize' do