From f6439a8820bcbee0cae7f3892407d2f17ceb390f Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Fri, 21 May 2021 21:46:09 -0700 Subject: [PATCH 1/2] Fixed memory bug: properly root repeated/map field when assigning. Previously the protobuf extension would not properly root memory from a repeated field or map when assigning to a message field (see the attached test case). This could cause crashes if the repeated field is subsequently accessed. --- ruby/ext/google/protobuf_c/map.c | 8 +++++--- ruby/ext/google/protobuf_c/map.h | 3 ++- ruby/ext/google/protobuf_c/message.c | 8 ++++---- ruby/ext/google/protobuf_c/protobuf.c | 10 ++++++++++ ruby/ext/google/protobuf_c/protobuf.h | 4 ++++ ruby/ext/google/protobuf_c/repeated_field.c | 6 ++++-- ruby/ext/google/protobuf_c/repeated_field.h | 3 ++- ruby/tests/basic.rb | 7 +++++++ ruby/tests/multi_level_nesting_test.rb | 20 -------------------- 9 files changed, 38 insertions(+), 31 deletions(-) delete mode 100644 ruby/tests/multi_level_nesting_test.rb diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c index 9d0b37e10fb7..d5b47e4e18c2 100644 --- a/ruby/ext/google/protobuf_c/map.c +++ b/ruby/ext/google/protobuf_c/map.c @@ -167,7 +167,8 @@ VALUE Map_deep_copy(VALUE obj) { new_arena_rb); } -const upb_map* Map_GetUpbMap(VALUE val, const upb_fielddef *field) { +const upb_map* Map_GetUpbMap(VALUE val, const upb_fielddef* field, + upb_arena* arena) { const upb_fielddef* key_field = map_field_key(field); const upb_fielddef* value_field = map_field_value(field); TypeInfo value_type_info = TypeInfo_get(value_field); @@ -189,6 +190,7 @@ const upb_map* Map_GetUpbMap(VALUE val, const upb_fielddef *field) { rb_raise(cTypeError, "Map value type has wrong message/enum class"); } + Arena_fuse(self->arena, arena); return self->map; } @@ -236,7 +238,7 @@ static VALUE Map_merge_into_self(VALUE _self, VALUE hashmap) { upb_msg *self_msg = Map_GetMutable(_self); size_t iter = UPB_MAP_BEGIN; - upb_arena_fuse(arena, Arena_get(other->arena)); + Arena_fuse(other->arena, arena); if (self->key_type != other->key_type || self->value_type_info.type != other->value_type_info.type || @@ -511,7 +513,7 @@ static VALUE Map_dup(VALUE _self) { upb_arena *arena = Arena_get(new_self->arena); upb_map *new_map = Map_GetMutable(new_map_rb); - upb_arena_fuse(arena, Arena_get(self->arena)); + Arena_fuse(self->arena, arena); while (upb_mapiter_next(self->map, &iter)) { upb_msgval key = upb_mapiter_key(self->map, iter); diff --git a/ruby/ext/google/protobuf_c/map.h b/ruby/ext/google/protobuf_c/map.h index 1b840c3da77e..411362cab3b5 100644 --- a/ruby/ext/google/protobuf_c/map.h +++ b/ruby/ext/google/protobuf_c/map.h @@ -44,7 +44,8 @@ VALUE Map_GetRubyWrapper(upb_map *map, upb_fieldtype_t key_type, // Gets the underlying upb_map for this Ruby map object, which must have // key/value type that match |field|. If this is not a map or the type doesn't // match, raises an exception. -const upb_map *Map_GetUpbMap(VALUE val, const upb_fielddef *field); +const upb_map *Map_GetUpbMap(VALUE val, const upb_fielddef *field, + upb_arena *arena); // Implements #inspect for this map by appending its contents to |b|. void Map_Inspect(StringBuilder *b, const upb_map *map, upb_fieldtype_t key_type, diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index c1b9b8633032..98f89e82771a 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -277,9 +277,9 @@ static void Message_setfield(upb_msg* msg, const upb_fielddef* f, VALUE val, upb_arena* arena) { upb_msgval msgval; if (upb_fielddef_ismap(f)) { - msgval.map_val = Map_GetUpbMap(val, f); + msgval.map_val = Map_GetUpbMap(val, f, arena); } else if (upb_fielddef_isseq(f)) { - msgval.array_val = RepeatedField_GetUpbArray(val, f); + msgval.array_val = RepeatedField_GetUpbArray(val, f, arena); } else { if (val == Qnil && (upb_fielddef_issubmsg(f) || upb_fielddef_realcontainingoneof(f))) { @@ -660,7 +660,7 @@ static VALUE Message_dup(VALUE _self) { // TODO(copy unknown fields?) // TODO(use official upb msg copy function) memcpy((upb_msg*)new_msg_self->msg, self->msg, size); - upb_arena_fuse(Arena_get(new_msg_self->arena), Arena_get(self->arena)); + Arena_fuse(self->arena, Arena_get(new_msg_self->arena)); return new_msg; } @@ -1314,7 +1314,7 @@ const upb_msg* Message_GetUpbMessage(VALUE value, const upb_msgdef* m, } Message* self = ruby_to_Message(value); - upb_arena_fuse(arena, Arena_get(self->arena)); + Arena_fuse(self->arena, arena); return self->msg; } diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index b657fa38639f..a61328b442e6 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -204,6 +204,16 @@ upb_arena *Arena_get(VALUE _arena) { return arena->arena; } +void Arena_fuse(VALUE _arena, upb_arena *other) { + Arena *arena; + TypedData_Get_Struct(_arena, Arena, &Arena_type, arena); + if (!upb_arena_fuse(arena->arena, other)) { + rb_raise(rb_eRuntimeError, + "Unable to fuse arenas. This should never happen since Ruby does " + "not use initial blocks"); + } +} + VALUE Arena_new() { return Arena_alloc(cArena); } diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 01f18fb3a43b..f7ac2049d709 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -55,6 +55,10 @@ const upb_fielddef* map_field_value(const upb_fielddef* field); VALUE Arena_new(); upb_arena *Arena_get(VALUE arena); +// Fuses this arena to another, throwing a Ruby exception if this is not +// possible. +void Arena_fuse(VALUE arena, upb_arena *other); + // Pins this Ruby object to the lifetime of this arena, so that as long as the // arena is alive this object will not be collected. // diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index da3e7ef0cdd7..1cc4915e06db 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/ruby/ext/google/protobuf_c/repeated_field.c @@ -149,7 +149,8 @@ VALUE RepeatedField_deep_copy(VALUE _self) { return new_rptfield; } -const upb_array* RepeatedField_GetUpbArray(VALUE val, const upb_fielddef *field) { +const upb_array* RepeatedField_GetUpbArray(VALUE val, const upb_fielddef* field, + upb_arena* arena) { RepeatedField* self; TypeInfo type_info = TypeInfo_get(field); @@ -167,6 +168,7 @@ const upb_array* RepeatedField_GetUpbArray(VALUE val, const upb_fielddef *field) rb_raise(cTypeError, "Repeated field array has wrong message/enum class"); } + Arena_fuse(self->arena, arena); return self->array; } @@ -412,7 +414,7 @@ static VALUE RepeatedField_dup(VALUE _self) { int size = upb_array_size(self->array); int i; - upb_arena_fuse(arena, Arena_get(self->arena)); + Arena_fuse(self->arena, arena); for (i = 0; i < size; i++) { upb_msgval msgval = upb_array_get(self->array, i); diff --git a/ruby/ext/google/protobuf_c/repeated_field.h b/ruby/ext/google/protobuf_c/repeated_field.h index 5a5959dd837b..e4ef25292437 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.h +++ b/ruby/ext/google/protobuf_c/repeated_field.h @@ -44,7 +44,8 @@ VALUE RepeatedField_GetRubyWrapper(upb_array* msg, TypeInfo type_info, // Gets the underlying upb_array for this Ruby RepeatedField object, which must // have a type that matches |f|. If this is not a repeated field or the type // doesn't match, raises an exception. -const upb_array* RepeatedField_GetUpbArray(VALUE value, const upb_fielddef* f); +const upb_array* RepeatedField_GetUpbArray(VALUE value, const upb_fielddef* f, + upb_arena* arena); // Implements #inspect for this repeated field by appending its contents to |b|. void RepeatedField_Inspect(StringBuilder* b, const upb_array* array, diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 8ddf72b81e4e..6beb4b75713e 100755 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -63,6 +63,13 @@ def test_issue_8311_crash end end + def test_issue_8559_crash + msg = TestMessage.new + msg.repeated_int32 = ::Google::Protobuf::RepeatedField.new(:int32, [1, 2, 3]) + GC.start(full_mark: true, immediate_sweep: true) + TestMessage.encode(msg) + end + def test_has_field m = TestSingularFields.new assert !m.has_singular_msg? diff --git a/ruby/tests/multi_level_nesting_test.rb b/ruby/tests/multi_level_nesting_test.rb deleted file mode 100644 index 7e5d0ec6b727..000000000000 --- a/ruby/tests/multi_level_nesting_test.rb +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/ruby - -# multi_level_nesting_test_pb.rb is in the same directory as this test. -$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__))) - -require 'test/unit' -require 'multi_level_nesting_test_pb' - -# -# Provide tests for having messages nested 3 levels deep -# -class MultiLevelNestingTest < Test::Unit::TestCase - - def test_levels_exist - assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function").msgclass - assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter").msgclass - assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter.Value").msgclass - end - -end From 6a1e47fdb25c3d7638117751b653f16c0c19bd32 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Fri, 21 May 2021 22:04:52 -0700 Subject: [PATCH 2/2] Add accidentally-deleted Ruby test. --- ruby/tests/multi_level_nesting_test.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 ruby/tests/multi_level_nesting_test.rb diff --git a/ruby/tests/multi_level_nesting_test.rb b/ruby/tests/multi_level_nesting_test.rb new file mode 100644 index 000000000000..7e5d0ec6b727 --- /dev/null +++ b/ruby/tests/multi_level_nesting_test.rb @@ -0,0 +1,20 @@ +#!/usr/bin/ruby + +# multi_level_nesting_test_pb.rb is in the same directory as this test. +$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__))) + +require 'test/unit' +require 'multi_level_nesting_test_pb' + +# +# Provide tests for having messages nested 3 levels deep +# +class MultiLevelNestingTest < Test::Unit::TestCase + + def test_levels_exist + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function").msgclass + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter").msgclass + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter.Value").msgclass + end + +end