Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed memory bug: properly root repeated/map field when assigning. #8639

Merged
merged 2 commits into from May 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions ruby/ext/google/protobuf_c/map.c
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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 ||
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion ruby/ext/google/protobuf_c/map.h
Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions ruby/ext/google/protobuf_c/message.c
Expand Up @@ -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))) {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
10 changes: 10 additions & 0 deletions ruby/ext/google/protobuf_c/protobuf.c
Expand Up @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions ruby/ext/google/protobuf_c/protobuf.h
Expand Up @@ -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.
//
Expand Down
6 changes: 4 additions & 2 deletions ruby/ext/google/protobuf_c/repeated_field.c
Expand Up @@ -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);

Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion ruby/ext/google/protobuf_c/repeated_field.h
Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions ruby/tests/basic.rb
Expand Up @@ -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?
Expand Down