Fixed memory bug: properly root repeated/map field when assigning. (#8639)
* 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.
* Add accidentally-deleted Ruby test.
diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c
index 9d0b37e..d5b47e4 100644
--- a/ruby/ext/google/protobuf_c/map.c
+++ b/ruby/ext/google/protobuf_c/map.c
@@ -167,7 +167,8 @@
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 @@
rb_raise(cTypeError, "Map value type has wrong message/enum class");
}
+ Arena_fuse(self->arena, arena);
return self->map;
}
@@ -236,7 +238,7 @@
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 @@
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 1b840c3..411362c 100644
--- a/ruby/ext/google/protobuf_c/map.h
+++ b/ruby/ext/google/protobuf_c/map.h
@@ -44,7 +44,8 @@
// 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 c1b9b86..98f89e8 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -277,9 +277,9 @@
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 @@
// 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 @@
}
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 b657fa3..a61328b 100644
--- a/ruby/ext/google/protobuf_c/protobuf.c
+++ b/ruby/ext/google/protobuf_c/protobuf.c
@@ -204,6 +204,16 @@
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 01f18fb..f7ac204 100644
--- a/ruby/ext/google/protobuf_c/protobuf.h
+++ b/ruby/ext/google/protobuf_c/protobuf.h
@@ -55,6 +55,10 @@
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 da3e7ef..1cc4915 100644
--- a/ruby/ext/google/protobuf_c/repeated_field.c
+++ b/ruby/ext/google/protobuf_c/repeated_field.c
@@ -149,7 +149,8 @@
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 @@
rb_raise(cTypeError, "Repeated field array has wrong message/enum class");
}
+ Arena_fuse(self->arena, arena);
return self->array;
}
@@ -412,7 +414,7 @@
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 5a5959d..e4ef252 100644
--- a/ruby/ext/google/protobuf_c/repeated_field.h
+++ b/ruby/ext/google/protobuf_c/repeated_field.h
@@ -44,7 +44,8 @@
// 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 8ddf72b..6beb4b7 100755
--- a/ruby/tests/basic.rb
+++ b/ruby/tests/basic.rb
@@ -63,6 +63,13 @@
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?
