diff options
| -rw-r--r-- | ruby/ext/google/protobuf_c/encode_decode.c | 9 | ||||
| -rw-r--r-- | ruby/ext/google/protobuf_c/protobuf.h | 14 | ||||
| -rw-r--r-- | ruby/ext/google/protobuf_c/storage.c | 44 |
3 files changed, 51 insertions, 16 deletions
diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index b9ecd456..0630f567 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -359,9 +359,12 @@ static void *oneofsubmsg_handler(void *closure, DEREF(msg, oneofdata->ofs, VALUE) = rb_class_new_instance(0, NULL, subklass); } - // Set the oneof case *after* allocating the new class instance -- see comment - // in layout_set() as to why. There are subtle interactions with possible GC - // points and oneof field type transitions. + // Set the oneof case *after* allocating the new class instance -- otherwise, + // if the Ruby GC is invoked as part of a call into the VM, it might invoke + // our mark routines, and our mark routines might see the case value + // indicating a VALUE is present and expect a valid VALUE. See comment in + // layout_set() for more detail: basically, the change to the value and the + // case must be atomic w.r.t. the Ruby VM. DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->oneof_case_num; diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 5b94ae26..d8a327aa 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -292,6 +292,15 @@ void native_slot_set(upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value); +// Atomically (with respect to Ruby VM calls) either update the value and set a +// oneof case, or do neither. If |case_memory| is null, then no case value is +// set. +void native_slot_set_value_and_case(upb_fieldtype_t type, + VALUE type_class, + void* memory, + VALUE value, + uint32_t* case_memory, + uint32_t case_number); VALUE native_slot_get(upb_fieldtype_t type, VALUE type_class, const void* memory); @@ -313,6 +322,11 @@ VALUE field_type_class(const upb_fielddef* field); #define MAP_KEY_FIELD 1 #define MAP_VALUE_FIELD 2 +// Oneof case slot value to indicate that no oneof case is set. The value `0` is +// safe because field numbers are used as case identifiers, and no field can +// have a number of 0. +#define ONEOF_CASE_NONE 0 + // These operate on a map field (i.e., a repeated field of submessages whose // submessage type is a map-entry msgdef). bool is_map_field(const upb_fielddef* field); diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 662101f8..5b1549d2 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -109,6 +109,17 @@ void native_slot_validate_string_encoding(upb_fieldtype_t type, VALUE value) { void native_slot_set(upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value) { + native_slot_set_value_and_case(type, type_class, memory, value, NULL, 0); +} + +void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, + void* memory, VALUE value, + uint32_t* case_memory, + uint32_t case_number) { + // Note that in order to atomically change the value in memory and the case + // value (w.r.t. Ruby VM calls), we must set the value at |memory| only after + // all Ruby VM calls are complete. The case is then set at the bottom of this + // function. switch (type) { case UPB_TYPE_FLOAT: if (!is_ruby_num(value)) { @@ -198,6 +209,10 @@ void native_slot_set(upb_fieldtype_t type, VALUE type_class, default: break; } + + if (case_memory != NULL) { + *case_memory = case_number; + } } VALUE native_slot_get(upb_fieldtype_t type, @@ -408,6 +423,13 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) { // We assign all value slots first, then pack the 'case' fields at the end, // since in the common case (modern 64-bit platform) these are 8 bytes and 4 // bytes respectively and we want to avoid alignment overhead. + // + // Note that we reserve 4 bytes (a uint32) per 'case' slot because the value + // space for oneof cases is conceptually as wide as field tag numbers. In + // practice, it's unlikely that a oneof would have more than e.g. 256 or 64K + // members (8 or 16 bits respectively), so conceivably we could assign + // consecutive case numbers and then pick a smaller oneof case slot size, but + // the complexity to implement this indirection is probably not worthwhile. upb_msg_oneof_iter oit; for (upb_msg_oneof_begin(&oit, msgdef); !upb_msg_oneof_done(&oit); @@ -576,7 +598,7 @@ void layout_set(MessageLayout* layout, if (upb_fielddef_containingoneof(field)) { if (val == Qnil) { // Assigning nil to a oneof field clears the oneof completely. - *oneof_case = 0; + *oneof_case = ONEOF_CASE_NONE; memset(memory, 0, NATIVE_SLOT_MAX_SIZE); } else { // The transition between field types for a single oneof (union) slot is @@ -587,18 +609,14 @@ void layout_set(MessageLayout* layout, // what it thinks is a primitive field but is actually a VALUE for the new // field type). // - // native_slot_set() has two parts: (i) conversion of some sort, and (ii) - // setting the in-memory content to the new value. It guarantees that all - // calls to the Ruby VM are completed before the memory slot is altered. - // // In order for the transition to be safe, the oneof case slot must be in - // sync with the value slot whenever the Ruby VM has been called. Because - // we are guaranteed that no Ruby VM calls occur after native_slot_set() - // alters the memory slot and before it returns, we set the oneof case - // immediately after native_slot_set() returns. - native_slot_set(upb_fielddef_type(field), field_type_class(field), - memory, val); - *oneof_case = upb_fielddef_number(field); + // sync with the value slot whenever the Ruby VM has been called. Thus, we + // use native_slot_set_value_and_case(), which ensures that both the value + // and case number are altered atomically (w.r.t. the Ruby VM). + native_slot_set_value_and_case( + upb_fielddef_type(field), field_type_class(field), + memory, val, + oneof_case, upb_fielddef_number(field)); } } else if (is_map_field(field)) { check_map_field_type(val, field); @@ -624,7 +642,7 @@ void layout_init(MessageLayout* layout, if (upb_fielddef_containingoneof(field)) { memset(memory, 0, NATIVE_SLOT_MAX_SIZE); - *oneof_case = 0; + *oneof_case = ONEOF_CASE_NONE; } else if (is_map_field(field)) { VALUE map = Qnil; |