Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ruby: encoding messages w/ message fields as JSON segfaults #283

Closed
wfarr opened this issue Apr 13, 2015 · 8 comments · Fixed by #284
Closed

Ruby: encoding messages w/ message fields as JSON segfaults #283

wfarr opened this issue Apr 13, 2015 · 8 comments · Fixed by #284

Comments

@wfarr
Copy link

wfarr commented Apr 13, 2015

We're currently testing w/ google-protobuf-3.0.0.alpha.2.0 (this appears to be the latest released version on rubygems.org) alongside grpc's ruby bindings.

Given a couple messages, defined as:

syntax = "proto3";
package = example;

message Foo {
       string foo = 1;
}

message Bar {
       string bar = 1;
       Foo foo = 2;
}

And some Ruby attempting to encode these messages as JSON:

# this does not blow up
foo = Example::Foo.new(foo: "this is okay")
Example::Foo.encode_json(foo)

# this very much blows up
Example::Bar.encode_json(Example::Bar.new)

# this also blows up
bar = Example::Bar.new(bar: "this is not okay")
Example::Bar.encode_json(bar)

# this blows up as well
foo = Example::Foo.new(foo: "womp womp")
bar = Example::Bar.new(bar: "this is not okay", foo: foo)
Example::Bar.encode_json(bar)

The failing examples as listed above all result in the same segmentation fault inside protobuf_c:

/usr/local/bundle/bin/rspec: [BUG] Segmentation fault at 0x0000000000001c
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]

-- Control frame information -----------------------------------------------
c:0001 p:0000 s:0002 E:001d98 TOP    [FINISH]


-- C level backtrace information -------------------------------------------
ruby(+0x1823bc) [0x7f375e46e3bc]
ruby(+0x1f4343) [0x7f375e4e0343]
ruby(rb_bug+0xb3) [0x7f375e4e1543]
ruby(+0xf923f) [0x7f375e3e523f]
/lib/x86_64-linux-gnu/libpthread.so.0(+0xf8d0) [0x7f375debb8d0]
/usr/local/bundle/extensions/x86_64-linux/2.1.0-static/google-protobuf-3.0.0.alpha.2.0/google/protobuf_c.so(upb_refcounted_unref+0) [0x7f3758783e90] upb.c:5781
/usr/local/bundle/extensions/x86_64-linux/2.1.0-static/google-protobuf-3.0.0.alpha.2.0/google/protobuf_c.so(Descriptor_free+0x67) [0x7f3758770c87] defs.c:253
ruby(+0x3b0df) [0x7f375e3270df]
ruby(rb_gc_call_finalizer_at_exit+0x2d9) [0x7f375e32d1d9]
ruby(ruby_cleanup+0x3df) [0x7f375e31211f]
ruby(ruby_run_node+0x36) [0x7f375e312486]
ruby(+0x226cb) [0x7f375e30e6cb]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f375d3e8b45] upb.h:6243
ruby(+0x226f9) [0x7f375e30e6f9]
@wfarr
Copy link
Author

wfarr commented Apr 13, 2015

I've just checked out master and run the tests in the ruby/ directory, and can confirm the tests do pass for me there.

I'm investigating what I can do to introduce a failing test case that reproduces my problem.

@wfarr
Copy link
Author

wfarr commented Apr 13, 2015

I'm now able to reproduce this on master with a small change. Here's the diff against master:

diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb
index 9d0f0a9..20a3c12 100644
--- a/ruby/tests/basic.rb
+++ b/ruby/tests/basic.rb
@@ -8,6 +8,19 @@ require 'test/unit'
 module BasicTest
   pool = Google::Protobuf::DescriptorPool.new
   pool.build do
+    add_message "Foo" do
+      optional :bar, :message, 1, "Bar"
+      repeated :baz, :message, 2, "Baz"
+    end
+
+    add_message "Bar" do
+      optional :msg, :string, 1
+    end
+
+    add_message "Baz" do
+      optional :msg, :string, 1
+    end
+
     add_message "TestMessage" do
       optional :optional_int32,  :int32,        1
       optional :optional_int64,  :int64,        2
@@ -84,6 +97,9 @@ module BasicTest
     end
   end

+  Foo = pool.lookup("Foo").msgclass
+  Bar = pool.lookup("Bar").msgclass
+  Baz = pool.lookup("Baz").msgclass
   TestMessage = pool.lookup("TestMessage").msgclass
   TestMessage2 = pool.lookup("TestMessage2").msgclass
   Recursive1 = pool.lookup("Recursive1").msgclass
@@ -1002,5 +1018,15 @@ module BasicTest
       m2 = MapMessage.decode_json(MapMessage.encode_json(m))
       assert m == m2
     end
+
+    def test_json_again
+      skip("Unimplemented") if RUBY_PLATFORM == "java"
+      bar = Bar.new(msg: "bar")
+      baz1 = Baz.new(msg: "baz")
+      baz2 = Baz.new(msg: "quux")
+      Foo.encode_json(Foo.new)
+      Foo.encode_json(Foo.new(bar: bar))
+      Foo.encode_json(Foo.new(bar: bar, baz: [baz1, baz2]))
+    end
   end
 end

Running the tests generates a segfault on program exit as demonstrated here.

@cfallin cfallin self-assigned this Apr 13, 2015
@cfallin
Copy link
Contributor

cfallin commented Apr 13, 2015

Thanks for reporting this -- I'll take a look right now.

@alindeman
Copy link

I believe it's a typo on this line: https://github.com/google/protobuf/blob/be89e626a6d61be50844ba78cfbff857a4c59c3b/ruby/ext/google/protobuf_c/defs.c#L253

AFAICT, the line should read self->json_serialize_handlers instead of self->pb_serialize_handlers

@cfallin
Copy link
Contributor

cfallin commented Apr 13, 2015

Yes, just found that too :-) Will send a PR shortly to fix.

@wfarr
Copy link
Author

wfarr commented Apr 13, 2015

Awesome. 😁

cfallin added a commit to cfallin/protobuf that referenced this issue Apr 13, 2015
@cfallin
Fixed issue protocolbuffers#283: crash in JSON handler cleanup.
e9abbd2 
Includes repro test case from @wfarr.
@cfallin cfallin mentioned this issue Apr 13, 2015
Merged
@cfallin
Copy link
Contributor

cfallin commented Apr 13, 2015

New version pushed to RubyGems as 3.0.0.alpha.3.1.pre.

@wfarr
Copy link
Author

wfarr commented Apr 14, 2015

Thanks much.

@cfallin cfallin removed their assignment May 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for issue 283: JSON handler cleanup typo causing segfault. cfallin/protobuf
3 participants
@wfarr @cfallin @alindeman