From dfa54577d65d7570593435318bb6aadc05c6bdf3 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 2 Mar 2021 12:34:41 -0800 Subject: [PATCH 1/2] [Ruby] Fixed SEGV when users pass nil messages. --- ruby/ext/google/protobuf_c/message.c | 4 +++- ruby/tests/basic.rb | 11 ++++++++--- ruby/tests/repeated_field_test.rb | 12 ------------ 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 77b0e8b9c175..aed84bdcd0e5 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -1248,7 +1248,9 @@ upb_msg* Message_deep_copy(const upb_msg* msg, const upb_msgdef* m, const upb_msg* Message_GetUpbMessage(VALUE value, const upb_msgdef* m, const char* name, upb_arena* arena) { - if (value == Qnil) return NULL; + if (value == Qnil) { + rb_raise(cTypeError, "nil message not allowed here."); + } VALUE klass = CLASS_OF(value); VALUE desc_rb = rb_ivar_get(klass, descriptor_instancevar_interned); diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 107084e66436..8ddf72b81e4e 100755 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -52,10 +52,15 @@ def test_issue_8311_crash outer = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Outer").msgclass - outer_proto = outer.new( + outer.new( inners: [] - ) - outer_proto['inners'].to_s + )['inners'].to_s + + assert_raise Google::Protobuf::TypeError do + outer.new( + inners: [nil] + ).to_s + end end def test_has_field diff --git a/ruby/tests/repeated_field_test.rb b/ruby/tests/repeated_field_test.rb index 6307447bc3f3..1df6e1d4f154 100755 --- a/ruby/tests/repeated_field_test.rb +++ b/ruby/tests/repeated_field_test.rb @@ -339,18 +339,6 @@ def test_collect! end end - def test_compact! - m = TestMessage.new - m.repeated_msg << TestMessage2.new(:foo => 1) - m.repeated_msg << nil - m.repeated_msg << TestMessage2.new(:foo => 2) - reference_arr = m.repeated_string.to_a - - check_self_modifying_method(m.repeated_string, reference_arr) do |arr| - arr.compact! - end - end - def test_delete m = TestMessage.new reference_arr = %w(foo bar baz) From 7d63b996e1b2b341f0a527d7e5ae0f153d595ce4 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 2 Mar 2021 15:20:06 -0800 Subject: [PATCH 2/2] Removed compatibility test that tries to append "nil" to repeated field. --- .../v3.0.0/tests/repeated_field_test.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb b/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb index b4a158f37ccd..4f70f52dc4a9 100755 --- a/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb +++ b/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb @@ -326,18 +326,6 @@ def test_collect! end end - def test_compact! - m = TestMessage.new - m.repeated_msg << TestMessage2.new(:foo => 1) - m.repeated_msg << nil - m.repeated_msg << TestMessage2.new(:foo => 2) - reference_arr = m.repeated_string.to_a - - check_self_modifying_method(m.repeated_string, reference_arr) do |arr| - arr.compact! - end - end - def test_delete m = TestMessage.new reference_arr = %w(foo bar baz)