Merge pull request #8313 from haberman/ruby-crash-fix
[Ruby] Bugfix for Message.[] for repeated or map fields.
diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index 22a21c1..f8661df 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -292,6 +292,35 @@
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));
@@ -350,36 +379,11 @@
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);
- }
}
}
@@ -866,7 +870,6 @@
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));
@@ -875,8 +878,7 @@
return Qnil;
}
- val = upb_msg_get(self->msg, field);
- return Convert_UpbToRuby(val, TypeInfo_get(field), self->arena);
+ return Message_getfield(_self, field);
}
/*
diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb
index 098ac41..107084e 100755
--- a/ruby/tests/basic.rb
+++ b/ruby/tests/basic.rb
@@ -31,6 +31,33 @@
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?
