Skip to content

Commit

Permalink
Fix ruby segment fault (protocolbuffers#3708)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
TeBoring authored Oct 4, 2017
1 parent d6c32a8 commit cd5f49d
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,8 @@ js/testproto_libs2.js

# Ignore the bazel symlinks
/bazel-*

# ruby test output
ruby/lib/
ruby/tests/generated_code_pb.rb
ruby/tests/test_import_pb.rb
1 change: 1 addition & 0 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,7 @@ ruby_EXTRA_DIST= \
ruby/src/main/java/google/ProtobufJavaService.java \
ruby/src/main/sentinel.proto \
ruby/tests/basic.rb \
ruby/tests/gc_test.rb \
ruby/tests/repeated_field_test.rb \
ruby/tests/stress.rb \
ruby/tests/generated_code.proto \
Expand Down
8 changes: 7 additions & 1 deletion ruby/Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
17 changes: 14 additions & 3 deletions ruby/ext/google/protobuf_c/defs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion ruby/ext/google/protobuf_c/protobuf.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
5 changes: 1 addition & 4 deletions ruby/ext/google/protobuf_c/protobuf.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
58 changes: 58 additions & 0 deletions ruby/tests/gc_test.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions ruby/travis-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit cd5f49d

Please sign in to comment.