From cd5f49d0942e19a5854a325941918fca02fdb409 Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Tue, 3 Oct 2017 17:28:49 -0700 Subject: Fix ruby segment fault (#3708) * Fix ruby segment fault 1) rb_ary_new cannot be called during allocate function. During allocate fucntion, the containing object hasn't been marked and rb_ary_new may invoke gc to collect containing object. 2) The global map should be marked before allocating it. Otherwise it may be garbage collected. * Add test * Remove commented code * Fix grammer error --- ruby/Rakefile | 8 ++++- ruby/ext/google/protobuf_c/defs.c | 17 ++++++++-- ruby/ext/google/protobuf_c/protobuf.c | 2 +- ruby/ext/google/protobuf_c/protobuf.h | 5 +-- ruby/tests/gc_test.rb | 58 +++++++++++++++++++++++++++++++++++ ruby/travis-test.sh | 1 + 6 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 ruby/tests/gc_test.rb (limited to 'ruby') diff --git a/ruby/Rakefile b/ruby/Rakefile index a329a777..e30a75a3 100644 --- a/ruby/Rakefile +++ b/ruby/Rakefile @@ -99,7 +99,13 @@ Gem::PackageTask.new(spec) do |pkg| end Rake::TestTask.new(:test => :build) do |t| - t.test_files = FileList["tests/*.rb"] + t.test_files = FileList["tests/*.rb"].exclude("tests/gc_test.rb") +end + +# gc_test needs to be split out to ensure the generated file hasn't been +# imported by other tests. +Rake::TestTask.new(:gc_test => :build) do |t| + t.test_files = FileList["tests/gc_test.rb"] end task :build => [:clean, :compile, :genproto] diff --git a/ruby/ext/google/protobuf_c/defs.c b/ruby/ext/google/protobuf_c/defs.c index 845c0225..34d9663a 100644 --- a/ruby/ext/google/protobuf_c/defs.c +++ b/ruby/ext/google/protobuf_c/defs.c @@ -228,7 +228,6 @@ DEFINE_CLASS(Descriptor, "Google::Protobuf::Descriptor"); void Descriptor_mark(void* _self) { Descriptor* self = _self; rb_gc_mark(self->klass); - rb_gc_mark(self->typeclass_references); } void Descriptor_free(void* _self) { @@ -283,7 +282,6 @@ VALUE Descriptor_alloc(VALUE klass) { self->pb_serialize_handlers = NULL; self->json_serialize_handlers = NULL; self->json_serialize_handlers_preserve = NULL; - self->typeclass_references = rb_ary_new(); return ret; } @@ -1635,7 +1633,7 @@ VALUE Builder_alloc(VALUE klass) { Builder* self = ALLOC(Builder); VALUE ret = TypedData_Wrap_Struct( klass, &_Builder_type, self); - self->pending_list = rb_ary_new(); + self->pending_list = Qnil; self->defs = NULL; return ret; } @@ -1645,11 +1643,24 @@ void Builder_register(VALUE module) { rb_define_alloc_func(klass, Builder_alloc); rb_define_method(klass, "add_message", Builder_add_message, 1); rb_define_method(klass, "add_enum", Builder_add_enum, 1); + rb_define_method(klass, "initialize", Builder_initialize, 0); rb_define_method(klass, "finalize_to_pool", Builder_finalize_to_pool, 1); cBuilder = klass; rb_gc_register_address(&cBuilder); } +/* + * call-seq: + * Builder.new(d) => builder + * + * Create a new message builder. + */ +VALUE Builder_initialize(VALUE _self) { + DEFINE_SELF(Builder, self, _self); + self->pending_list = rb_ary_new(); + return Qnil; +} + /* * call-seq: * Builder.add_message(name, &block) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index 7cde4aec..c7750c44 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -110,6 +110,6 @@ void Init_protobuf_c() { kRubyStringASCIIEncoding = rb_usascii_encoding(); kRubyString8bitEncoding = rb_ascii8bit_encoding(); - upb_def_to_ruby_obj_map = rb_hash_new(); rb_gc_register_address(&upb_def_to_ruby_obj_map); + upb_def_to_ruby_obj_map = rb_hash_new(); } diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index f4b110fe..21c7f001 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -116,10 +116,6 @@ struct Descriptor { const upb_handlers* pb_serialize_handlers; const upb_handlers* json_serialize_handlers; const upb_handlers* json_serialize_handlers_preserve; - // Handlers hold type class references for sub-message fields directly in some - // cases. We need to keep these rooted because they might otherwise be - // collected. - VALUE typeclass_references; }; struct FieldDescriptor { @@ -280,6 +276,7 @@ void Builder_free(void* _self); VALUE Builder_alloc(VALUE klass); void Builder_register(VALUE module); Builder* ruby_to_Builder(VALUE value); +VALUE Builder_initialize(VALUE _self); VALUE Builder_add_message(VALUE _self, VALUE name); VALUE Builder_add_enum(VALUE _self, VALUE name); VALUE Builder_finalize_to_pool(VALUE _self, VALUE pool_rb); diff --git a/ruby/tests/gc_test.rb b/ruby/tests/gc_test.rb new file mode 100644 index 00000000..f3470cca --- /dev/null +++ b/ruby/tests/gc_test.rb @@ -0,0 +1,58 @@ +#!/usr/bin/ruby +# +# generated_code.rb is in the same directory as this test. +$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__))) + +old_gc = GC.stress +GC.stress = 0x01 | 0x04 +require 'generated_code_pb' +GC.stress = old_gc + +require 'test/unit' + +class GCTest < Test::Unit::TestCase + def get_msg + A::B::C::TestMessage.new( + :optional_int32 => 1, + :optional_int64 => 1, + :optional_uint32 => 1, + :optional_uint64 => 1, + :optional_bool => true, + :optional_double => 1.0, + :optional_float => 1.0, + :optional_string => "a", + :optional_bytes => "b", + :optional_enum => A::B::C::TestEnum::A, + :optional_msg => A::B::C::TestMessage.new(), + :repeated_int32 => [1], + :repeated_int64 => [1], + :repeated_uint32 => [1], + :repeated_uint64 => [1], + :repeated_bool => [true], + :repeated_double => [1.0], + :repeated_float => [1.0], + :repeated_string => ["a"], + :repeated_bytes => ["b"], + :repeated_enum => [A::B::C::TestEnum::A], + :repeated_msg => [A::B::C::TestMessage.new()], + :map_int32_string => {1 => "a"}, + :map_int64_string => {1 => "a"}, + :map_uint32_string => {1 => "a"}, + :map_uint64_string => {1 => "a"}, + :map_bool_string => {true => "a"}, + :map_string_string => {"a" => "a"}, + :map_string_msg => {"a" => A::B::C::TestMessage.new()}, + :map_string_int32 => {"a" => 1}, + :map_string_bool => {"a" => true}, + ) + end + def test_generated_msg + old_gc = GC.stress + GC.stress = 0x01 | 0x04 + from = get_msg + data = A::B::C::TestMessage.encode(from) + to = A::B::C::TestMessage.decode(data) + GC.stress = old_gc + puts "passed" + end +end diff --git a/ruby/travis-test.sh b/ruby/travis-test.sh index 52ea81b6..cbe7cd98 100755 --- a/ruby/travis-test.sh +++ b/ruby/travis-test.sh @@ -20,6 +20,7 @@ test_version() { git clean -f && \ gem install bundler && bundle && \ rake test && + rake gc_test && cd ../conformance && make test_ruby && cd ../ruby/compatibility_tests/v3.0.0 && ./test.sh" fi -- cgit v1.2.3