diff --git a/hpb/extension.h b/hpb/extension.h index 2cacc765426c0..722480c5f92b4 100644 --- a/hpb/extension.h +++ b/hpb/extension.h @@ -134,8 +134,9 @@ class ExtensionRegistry { void AddExtension(const ExtensionIdentifier& id) { if (registry_) { auto* extension = id.mini_table_ext(); - bool success = upb_ExtensionRegistry_AddArray(registry_, &extension, 1); - if (!success) { + upb_ExtensionRegistryStatus status = + upb_ExtensionRegistry_AddArray(registry_, &extension, 1); + if (status != kUpb_ExtensionRegistryStatus_Ok) { registry_ = nullptr; } } diff --git a/python/google/protobuf/internal/message_factory_test.py b/python/google/protobuf/internal/message_factory_test.py index d32b046077965..5c9e4bf8ea6b5 100644 --- a/python/google/protobuf/internal/message_factory_test.py +++ b/python/google/protobuf/internal/message_factory_test.py @@ -179,14 +179,19 @@ def testDuplicateExtensionNumber(self): type_name='Duplicate', extendee='Container', ) + if api_implementation.Type() == 'upb': with self.assertRaisesRegex( TypeError, - # TODO - b/380939902: Figure out why this fails with an OOM message. - "Couldn't build proto file into descriptor pool: out of memory", + "Couldn't build proto file into descriptor pool: " + 'duplicate extension entry', ): pool.Add(f) else: + # TODO: b/381131694 - Ensure conformance between upb/c++/python. + # C++ and pure Python implementations should raise an error when adding a + # duplicate extension number. There doesn't seem to be a benefit to failing + # only when GetMessageClassesForFiles is called. pool.Add(f) with self.assertRaises(Exception) as cm: diff --git a/upb/mini_table/extension_registry.c b/upb/mini_table/extension_registry.c index 65ac7873256f1..32b07b8599972 100644 --- a/upb/mini_table/extension_registry.c +++ b/upb/mini_table/extension_registry.c @@ -11,6 +11,7 @@ #include #include +#include "upb/hash/common.h" #include "upb/hash/str_table.h" #include "upb/mem/arena.h" #include "upb/mini_table/extension.h" @@ -39,24 +40,32 @@ upb_ExtensionRegistry* upb_ExtensionRegistry_New(upb_Arena* arena) { return r; } -UPB_API bool upb_ExtensionRegistry_Add(upb_ExtensionRegistry* r, - const upb_MiniTableExtension* e) { +UPB_API upb_ExtensionRegistryStatus upb_ExtensionRegistry_Add( + upb_ExtensionRegistry* r, const upb_MiniTableExtension* e) { char buf[EXTREG_KEY_SIZE]; extreg_key(buf, e->UPB_PRIVATE(extendee), upb_MiniTableExtension_Number(e)); - if (upb_strtable_lookup2(&r->exts, buf, EXTREG_KEY_SIZE, NULL)) return false; - return upb_strtable_insert(&r->exts, buf, EXTREG_KEY_SIZE, - upb_value_constptr(e), r->arena); + + if (upb_strtable_lookup2(&r->exts, buf, EXTREG_KEY_SIZE, NULL)) { + return kUpb_ExtensionRegistryStatus_DuplicateEntry; + } + + if (!upb_strtable_insert(&r->exts, buf, EXTREG_KEY_SIZE, + upb_value_constptr(e), r->arena)) { + return kUpb_ExtensionRegistryStatus_OutOfMemory; + } + return kUpb_ExtensionRegistryStatus_Ok; } -bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r, - const upb_MiniTableExtension** e, - size_t count) { +upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray( + upb_ExtensionRegistry* r, const upb_MiniTableExtension** e, size_t count) { const upb_MiniTableExtension** start = e; const upb_MiniTableExtension** end = UPB_PTRADD(e, count); + upb_ExtensionRegistryStatus status = kUpb_ExtensionRegistryStatus_Ok; for (; e < end; e++) { - if (!upb_ExtensionRegistry_Add(r, *e)) goto failure; + status = upb_ExtensionRegistry_Add(r, *e); + if (status != kUpb_ExtensionRegistryStatus_Ok) goto failure; } - return true; + return kUpb_ExtensionRegistryStatus_Ok; failure: // Back out the entries previously added. @@ -67,7 +76,8 @@ bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r, upb_MiniTableExtension_Number(ext)); upb_strtable_remove2(&r->exts, buf, EXTREG_KEY_SIZE, NULL); } - return false; + UPB_ASSERT(status != kUpb_ExtensionRegistryStatus_Ok); + return status; } #ifdef UPB_LINKARR_DECLARE @@ -80,7 +90,9 @@ bool upb_ExtensionRegistry_AddAllLinkedExtensions(upb_ExtensionRegistry* r) { for (const upb_MiniTableExtension* p = start; p < stop; p++) { // Windows can introduce zero padding, so we have to skip zeroes. if (upb_MiniTableExtension_Number(p) != 0) { - if (!upb_ExtensionRegistry_Add(r, p)) return false; + if (upb_ExtensionRegistry_Add(r, p) != kUpb_ExtensionRegistryStatus_Ok) { + return false; + } } } return true; diff --git a/upb/mini_table/extension_registry.h b/upb/mini_table/extension_registry.h index 9f5f81a33c563..6d294a8a6fbca 100644 --- a/upb/mini_table/extension_registry.h +++ b/upb/mini_table/extension_registry.h @@ -8,6 +8,9 @@ #ifndef UPB_MINI_TABLE_EXTENSION_REGISTRY_H_ #define UPB_MINI_TABLE_EXTENSION_REGISTRY_H_ +#include +#include + #include "upb/mem/arena.h" #include "upb/mini_table/extension.h" #include "upb/mini_table/message.h" @@ -55,21 +58,25 @@ extern "C" { typedef struct upb_ExtensionRegistry upb_ExtensionRegistry; +typedef enum { + kUpb_ExtensionRegistryStatus_Ok = 0, + kUpb_ExtensionRegistryStatus_DuplicateEntry = 1, + kUpb_ExtensionRegistryStatus_OutOfMemory = 2, +} upb_ExtensionRegistryStatus; + // Creates a upb_ExtensionRegistry in the given arena. // The arena must outlive any use of the extreg. UPB_API upb_ExtensionRegistry* upb_ExtensionRegistry_New(upb_Arena* arena); -UPB_API bool upb_ExtensionRegistry_Add(upb_ExtensionRegistry* r, - const upb_MiniTableExtension* e); +UPB_API upb_ExtensionRegistryStatus upb_ExtensionRegistry_Add( + upb_ExtensionRegistry* r, const upb_MiniTableExtension* e); // Adds the given extension info for the array |e| of size |count| into the // registry. If there are any errors, the entire array is backed out. // The extensions must outlive the registry. // Possible errors include OOM or an extension number that already exists. -// TODO: There is currently no way to know the exact reason for failure. -bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r, - const upb_MiniTableExtension** e, - size_t count); +upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray( + upb_ExtensionRegistry* r, const upb_MiniTableExtension** e, size_t count); #ifdef UPB_LINKARR_DECLARE diff --git a/upb/reflection/file_def.c b/upb/reflection/file_def.c index 5a51deecf947e..27e7f18209073 100644 --- a/upb/reflection/file_def.c +++ b/upb/reflection/file_def.c @@ -453,8 +453,15 @@ void _upb_FileDef_Create(upb_DefBuilder* ctx, } if (file->ext_count) { - bool ok = upb_ExtensionRegistry_AddArray( + upb_ExtensionRegistryStatus status = upb_ExtensionRegistry_AddArray( _upb_DefPool_ExtReg(ctx->symtab), file->ext_layouts, file->ext_count); - if (!ok) _upb_DefBuilder_OomErr(ctx); + if (status != kUpb_ExtensionRegistryStatus_Ok) { + if (status == kUpb_ExtensionRegistryStatus_OutOfMemory) { + _upb_DefBuilder_OomErr(ctx); + } + + UPB_ASSERT(status == kUpb_ExtensionRegistryStatus_DuplicateEntry); + _upb_DefBuilder_Errf(ctx, "duplicate extension entry"); + } } }