Merge pull request #8363 from haberman/ruby-nil-fix

Fixed SEGV when users pass nil messages. This also disallows `nil` in RepeatedField or Map.
protocolbuffers · Mar 3, 2021 · cf7d81f · cf7d81f
2 parents 1add7a7 + 7d63b99
commit cf7d81f
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 28 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
@@ -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)

diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
@@ -1250,7 +1250,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
@@ -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
@@ -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)