Skip to content

Conversation

@casperisfine
Copy link

rb_ivar_set may trigger GC, and if it does then we'll try to mark an uninitialized buffer and that will segfault because b->head == NULL.

Should fix:

[BUG] Segmentation fault at 0x0000000000000020

/tmp/bundle/ruby/3.2.0/gems/msgpack-1.6.0/lib/msgpack/msgpack.so(msgpack_buffer_mark+0x20) [0x7f8f07e72920] buffer.c:122
/tmp/bundle/ruby/3.2.0/gems/msgpack-1.6.0/lib/msgpack/msgpack.so(msgpack_buffer_mark) (null):0
/usr/local/ruby/bin/ruby(gc_mark_children+0x817) [0x55f59a9a8447] gc.c:7327
/usr/local/ruby/bin/ruby(gc_mark_stacked_objects+0xe) [0x55f59a9a9959] gc.c:7447
/usr/local/ruby/bin/ruby(gc_mark_stacked_objects_all) gc.c:7487

cc @peterzhu2118

@casperisfine
Copy link
Author

NB: I suspect we might have another bug, because I can't find any place allocating standalone buffers in the application that experienced this crash.

So a similar bug might be possible with "shared" buffers, but I couldn't find a repro yet.

@casperisfine
Copy link
Author

Yeah, I confirm this doesn't fix the application in question. So there is another case where we might mark uninitialized buffers.

@casperisfine
Copy link
Author

So I tried 906fa01 and it still fail with the same error. If the line numbers in the stacktrace are to be believed we fail on:

while(c != &b->tail) {

Which makes very little sense as it would suggest b is 0x20, but then we'd have failed dereferencing it before:

    if(b->head) {
        msgpack_buffer_chunk_t* c = b->head;
        while(c != &b->tail) {
            rb_gc_mark(c->mapped_string);
            c = c->next;
        }
        rb_gc_mark(c->mapped_string);
    }

I'll keep digging.

@casperisfine
Copy link
Author

Interestingly in -O0 I get a sligthly different crash:

[BUG] Segmentation fault at 0x0000000000000110

/usr/local/ruby/bin/ruby(RVALUE_MARKED+0x34) [0x565159b5079f] gc.c:1677
/usr/local/ruby/bin/ruby(gc_mark_set) gc.c:6940
/usr/local/ruby/bin/ruby(gc_mark_ptr) gc.c:7054
msgpack-ruby-92bb8a87f89e/lib/msgpack/msgpack.so(msgpack_buffer_mark+0x8d) [0x7fbd56cf8080] buffer.c:131
gems/msgpack-ruby-92bb8a87f89e/lib/msgpack/msgpack.so(msgpack_buffer_mark) (null):0

Doesn't make much more sense but...

@casperisfine
Copy link
Author

casperisfine commented Mar 1, 2023

Another way it crash:

[BUG] Segmentation fault at 0x0000000000000018
msgpack-ruby-92bb8a87f89e/lib/msgpack/msgpack.so(msgpack_buffer_destroy+0x22) [0x7f2339df2f12] buffer.c:86
msgpack-ruby-92bb8a87f89e/lib/msgpack/msgpack.so(msgpack_buffer_destroy) (null):0
msgpack-ruby-92bb8a87f89e/lib/msgpack/msgpack.so(Buffer_free+0x2b) [0x7f2339df4e51] buffer_class.c:50
/usr/local/ruby/bin/ruby(run_final+0xf) [0x55d8c3d6a20e] gc.c:4398
/usr/local/ruby/bin/ruby(finalize_list) gc.c:4417
/usr/local/ruby/bin/ruby(finalize_deferred_heap_pages) gc.c:4446
/usr/local/ruby/bin/ruby(rb_objspace_call_finalizer+0x33d) [0x55d8c3d7c5cd] gc.c:4583

@casperisfine
Copy link
Author

Ok, @peterzhu2118 managed to figure it out by inspecting the core dump. The problem was a name clash with another gem: https://github.com/Shopify/stack_frames/blob/154a360cac62a247fe45a56c698814cf1274c302/ext/stack_frames/buffer.c#L36

const rb_data_type_t buffer_data_type is public and msgpack was loaded first, so that other gem was creating T_DATA with a totally different layout, but using buffer_mark from Message Pack.

The fix is to hide this symbol, as it has no need to be public.

The original bug I found is legit though, so I'll cleanup with PR to just be focused on that one bug I found, and I'll open another one to hide the symbols.

`rb_ivar_set` may trigger GC, and if it does then we'll try to
mark an uninitialized buffer and that will segfault because
`b->head == NULL`.
@byroot byroot merged commit da7cbf8 into msgpack:master Mar 1, 2023
casperisfine pushed a commit to Shopify/stack_frames that referenced this pull request Mar 1, 2023
Ref: msgpack/msgpack-ruby#314 (comment)

We experienced some weird segfault in `msgpack_buffer_mark` we couldn't explain.

Until we noticed that the object being marked wasn't a `MessagePack::Buffer` but
a `StackFrames::Buffer`.

Both `msgpack` and `stackframes` were using the same symbol name to declare
their respective buffer type, and the msgpack one was registered first.

This is the counterpart of msgpack/msgpack-ruby#317
and generally speaking all gems should compile with `-fvisibility=hidden`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants