diff --git a/upb/reflection/field_def.c b/upb/reflection/field_def.c index f9b667fe28..cc509dd6cd 100644 --- a/upb/reflection/field_def.c +++ b/upb/reflection/field_def.c @@ -638,8 +638,7 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix, upb_OneofDef* oneof = (upb_OneofDef*)upb_MessageDef_Oneof(m, oneof_index); f->scope.oneof = oneof; - bool ok = _upb_OneofDef_Insert(oneof, f, name.data, name.size, ctx->arena); - if (!ok) _upb_DefBuilder_OomErr(ctx); + _upb_OneofDef_Insert(ctx, oneof, f, name.data, name.size); } UPB_DEF_SET_OPTIONS(f->opts, FieldDescriptorProto, FieldOptions, field_proto); diff --git a/upb/reflection/oneof_def.c b/upb/reflection/oneof_def.c index 767bdc49cf..ba198924cd 100644 --- a/upb/reflection/oneof_def.c +++ b/upb/reflection/oneof_def.c @@ -115,15 +115,29 @@ const upb_FieldDef* upb_OneofDef_LookupNumber(const upb_OneofDef* o, : NULL; } -bool _upb_OneofDef_Insert(upb_OneofDef* o, const upb_FieldDef* f, - const char* name, size_t size, upb_Arena* a) { +void _upb_OneofDef_Insert(upb_DefBuilder* ctx, upb_OneofDef* o, + const upb_FieldDef* f, const char* name, + size_t size) { o->field_count++; if (_upb_FieldDef_IsProto3Optional(f)) o->synthetic = true; const int number = upb_FieldDef_Number(f); const upb_value v = upb_value_constptr(f); - return upb_inttable_insert(&o->itof, number, v, a) && - upb_strtable_insert(&o->ntof, name, size, v, a); + + // TODO(salo): This lookup is unfortunate because we also perform it when + // inserting into the message's table. Unfortunately that step occurs after + // this one and moving things around could be tricky so let's leave it for + // a future refactoring. + const bool exists = upb_inttable_lookup(&o->itof, number, NULL); + if (UPB_UNLIKELY(exists)) { + _upb_DefBuilder_Errf(ctx, "oneof fields have the same number (%d)", number); + } + + const bool ok = upb_inttable_insert(&o->itof, number, v, ctx->arena) && + upb_strtable_insert(&o->ntof, name, size, v, ctx->arena); + if (UPB_UNLIKELY(!ok)) { + _upb_DefBuilder_OomErr(ctx); + } } // Returns the synthetic count. diff --git a/upb/reflection/oneof_def_internal.h b/upb/reflection/oneof_def_internal.h index 3a98c6739c..029f1abf37 100644 --- a/upb/reflection/oneof_def_internal.h +++ b/upb/reflection/oneof_def_internal.h @@ -38,8 +38,8 @@ extern "C" { #endif upb_OneofDef* _upb_OneofDef_At(const upb_OneofDef* o, int i); -bool _upb_OneofDef_Insert(upb_OneofDef* o, const upb_FieldDef* f, - const char* name, size_t size, upb_Arena* a); +void _upb_OneofDef_Insert(upb_DefBuilder* ctx, upb_OneofDef* o, + const upb_FieldDef* f, const char* name, size_t size); // Allocate and initialize an array of |n| oneof defs owned by |m|. upb_OneofDef* _upb_OneofDefs_New( diff --git a/upb/util/def_to_proto_test.cc b/upb/util/def_to_proto_test.cc index e5d4b37086..2b3f5e75f2 100644 --- a/upb/util/def_to_proto_test.cc +++ b/upb/util/def_to_proto_test.cc @@ -176,6 +176,19 @@ TEST(FuzzTest, EditionEmbeddedNull) { ParseTextProtoOrDie(R"pb(file { name: "n" edition: "\000" })pb")); } +TEST(FuzzTest, DuplicateOneofIndex) { + RoundTripDescriptor(ParseTextProtoOrDie( + R"pb(file { + name: "F" + message_type { + name: "M" + oneof_decl { name: "O" } + field { name: "f1" number: 1 type: TYPE_INT32 oneof_index: 0 } + field { name: "f2" number: 1 type: TYPE_INT32 oneof_index: 0 } + } + })pb")); +} + TEST(FuzzTest, NanValue) { RoundTripDescriptor(ParseTextProtoOrDie( R"pb(file {