Skip to content

Commit 4e2a755

Browse files
authored
Merge pull request #405 from Watson1978/fix/recursive-ext-stack-underflow
Fix SEGV on stack depth underflow in recursive extension unpacking
2 parents 29d98d3 + 7ff80ad commit 4e2a755

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

ext/msgpack/unpacker.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,15 @@ static inline int read_raw_body_begin(msgpack_unpacker_t* uk, int raw_type)
386386
* instead of raising StackError like every other container type. */
387387
return PRIMITIVE_STACK_TOO_DEEP;
388388
}
389+
size_t barrier_depth = uk->stack.depth;
389390
int raised;
390391
obj = protected_proc_call(proc, 1, &uk->self, &raised);
391-
msgpack_unpacker_stack_pop(uk);
392+
393+
/* The user proc can drive the unpacker itself (Unpacker#read, #skip,
394+
* or a rescued error) and leave stack.depth anywhere, including 0.
395+
* Restore it to just below the barrier we pushed instead of an
396+
* unconditional decrement, which would underflow to SIZE_MAX. */
397+
uk->stack.depth = barrier_depth - 1;
392398

393399
if (raised) {
394400
uk->last_object = rb_errinfo();

spec/factory_spec.rb

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,48 @@ class << Symbol
699699
# recursed unbounded in C and crashed the VM (SIGSEGV) rather than raising.
700700
expect { factory.load(payload) }.to raise_error(MessagePack::StackError)
701701
end
702+
703+
it 'does not corrupt the stack when a recursive unpacker leaves the depth at zero' do
704+
# A recursive proc that rescues an inner read error, or that calls #skip,
705+
# can drive stack.depth down to 0 before read_raw_body_begin pops its
706+
# barrier. The unconditional pop then underflowed depth to SIZE_MAX and
707+
# read/wrote out-of-bounds stack entries (SIGSEGV). The payloads leave no
708+
# trailing bytes, so a fixed unpacker returns without raising.
709+
skip if IS_JRUBY
710+
711+
rescuing = MessagePack::Factory.new
712+
rescuing.register_type(0x01, Class.new,
713+
packer: ->(_obj, packer) { packer.write(nil) },
714+
unpacker: ->(u) {
715+
begin
716+
u.read
717+
rescue MessagePack::MalformedFormatError, EOFError
718+
nil
719+
end
720+
},
721+
recursive: true,
722+
)
723+
724+
skipping = MessagePack::Factory.new
725+
skipping.register_type(0x02, Class.new,
726+
packer: ->(_obj, packer) { packer.write(nil) },
727+
unpacker: ->(u) { u.skip },
728+
recursive: true,
729+
)
730+
731+
payloads = [
732+
[rescuing, "\xd4\x01\xc1".b], # fixext1 type=1, then an invalid byte
733+
[rescuing, "\xd4\x01".b], # fixext1 type=1, then truncated (EOF)
734+
[skipping, "\xd4\x02\x91\x2a".b], # fixext1 type=2, fixarray(1), 42
735+
]
736+
payloads.each do |factory, bytes|
737+
100.times { factory.unpack(bytes) }
738+
end
739+
740+
# The stack is intact: a fresh unpack still decodes correctly.
741+
expect(rescuing.unpack(MessagePack.pack([1, 2, 3]))).to eq([1, 2, 3])
742+
expect(skipping.unpack(MessagePack.pack("ok"))).to eq("ok")
743+
end
702744
end
703745

704746
describe 'memsize' do

0 commit comments

Comments
 (0)