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.

Sign up for GitHub

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

Jump to bottom

[Ruby] Bugfix for Message.[] for repeated or map fields. #8313

Merged
merged 1 commit into from Feb 19, 2021
Merged

[Ruby] Bugfix for Message.[] for repeated or map fields. #8313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 32 additions & 30 deletions ruby/ext/google/protobuf_c/message.c
View file
Open in desktop
Expand Up @@ -292,6 +292,35 @@ static void Message_setfield(upb_msg* msg, const upb_fielddef* f, VALUE val,
upb_msg_set(msg, f, msgval, arena);
}

static VALUE Message_getfield(VALUE _self, const upb_fielddef* f) {
Message* self = ruby_to_Message(_self);
// This is a special-case: upb_msg_mutable() for map & array are logically
// const (they will not change what is serialized) but physically
// non-const, as they do allocate a repeated field or map. The logical
// constness means it's ok to do even if the message is frozen.
upb_msg *msg = (upb_msg*)self->msg;
upb_arena *arena = Arena_get(self->arena);
if (upb_fielddef_ismap(f)) {
upb_map *map = upb_msg_mutable(msg, f, arena).map;
const upb_fielddef *key_f = map_field_key(f);
const upb_fielddef *val_f = map_field_value(f);
upb_fieldtype_t key_type = upb_fielddef_type(key_f);
TypeInfo value_type_info = TypeInfo_get(val_f);
return Map_GetRubyWrapper(map, key_type, value_type_info, self->arena);
} else if (upb_fielddef_isseq(f)) {
upb_array *arr = upb_msg_mutable(msg, f, arena).array;
return RepeatedField_GetRubyWrapper(arr, TypeInfo_get(f), self->arena);
} else if (upb_fielddef_issubmsg(f)) {
if (!upb_msg_has(self->msg, f)) return Qnil;
upb_msg *submsg = upb_msg_mutable(msg, f, arena).msg;
const upb_msgdef *m = upb_fielddef_msgsubdef(f);
return Message_GetRubyWrapper(submsg, m, self->arena);
} else {
upb_msgval msgval = upb_msg_get(self->msg, f);
return Convert_UpbToRuby(msgval, TypeInfo_get(f), self->arena);
}
}

static VALUE Message_field_accessor(VALUE _self, const upb_fielddef* f,
int accessor_type, int argc, VALUE* argv) {
upb_arena *arena = Arena_get(Message_GetArena(_self));
Expand Down Expand Up @@ -350,36 +379,11 @@ static VALUE Message_field_accessor(VALUE _self, const upb_fielddef* f,
return INT2NUM(msgval.int32_val);
}
}
case METHOD_GETTER: {
Message* self = ruby_to_Message(_self);
// This is a special-case: upb_msg_mutable() for map & array are logically
// const (they will not change what is serialized) but physically
// non-const, as they do allocate a repeated field or map. The logical
// constness means it's ok to do even if the message is frozen.
upb_msg *msg = (upb_msg*)self->msg;
if (upb_fielddef_ismap(f)) {
upb_map *map = upb_msg_mutable(msg, f, arena).map;
const upb_fielddef *key_f = map_field_key(f);
const upb_fielddef *val_f = map_field_value(f);
upb_fieldtype_t key_type = upb_fielddef_type(key_f);
TypeInfo value_type_info = TypeInfo_get(val_f);
return Map_GetRubyWrapper(map, key_type, value_type_info, self->arena);
} else if (upb_fielddef_isseq(f)) {
upb_array *arr = upb_msg_mutable(msg, f, arena).array;
return RepeatedField_GetRubyWrapper(arr, TypeInfo_get(f), self->arena);
} else if (upb_fielddef_issubmsg(f)) {
if (!upb_msg_has(self->msg, f)) return Qnil;
upb_msg *submsg = upb_msg_mutable(msg, f, arena).msg;
const upb_msgdef *m = upb_fielddef_msgsubdef(f);
return Message_GetRubyWrapper(submsg, m, self->arena);
} else {
upb_msgval msgval = upb_msg_get(self->msg, f);
return Convert_UpbToRuby(msgval, TypeInfo_get(f), self->arena);
}
case METHOD_GETTER:
return Message_getfield(_self, f);
default:
rb_raise(rb_eRuntimeError, "Internal error, no such accessor: %d",
accessor_type);
}
}
}

Expand Down Expand Up @@ -866,7 +870,6 @@ static VALUE Message_freeze(VALUE _self) {
static VALUE Message_index(VALUE _self, VALUE field_name) {
Message* self = ruby_to_Message(_self);
const upb_fielddef* field;
upb_msgval val;

Check_Type(field_name, T_STRING);
field = upb_msgdef_ntofz(self->msgdef, RSTRING_PTR(field_name));
Expand All @@ -875,8 +878,7 @@ static VALUE Message_index(VALUE _self, VALUE field_name) {
return Qnil;
}

val = upb_msg_get(self->msg, field);
return Convert_UpbToRuby(val, TypeInfo_get(field), self->arena);
return Message_getfield(_self, field);
}

/*
Expand Down
27 changes: 27 additions & 0 deletions ruby/tests/basic.rb
View file
Open in desktop
Expand Up @@ -31,6 +31,33 @@ def proto_module
end
include CommonTests

def test_issue_8311_crash
Google::Protobuf::DescriptorPool.generated_pool.build do
add_file("inner.proto", :syntax => :proto3) do
add_message "Inner" do
# Removing either of these fixes the segfault.
optional :foo, :string, 1
optional :bar, :string, 2
end
end
end

Google::Protobuf::DescriptorPool.generated_pool.build do
add_file("outer.proto", :syntax => :proto3) do
add_message "Outer" do
repeated :inners, :message, 1, "Inner"
end
end
end

outer = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Outer").msgclass

outer_proto = outer.new(
inners: []
)
outer_proto['inners'].to_s
end

def test_has_field
m = TestSingularFields.new
assert !m.has_singular_msg?
Expand Down