From a79fbc9d32723a383813f6be4791ddaed09a8d6c Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 2 Dec 2024 13:43:14 -0800 Subject: [PATCH] Fixed comparison of empty repeated/map extensions. Repeated/map extensions are semantically equivalent to an extension that is not present at all. We had code paths that were treating them differently, which led to incorrect results. In particular, we were considering `{.repeated_ext = []}` to be different from `{}` when comparing with `upb_Message_IsEqual()`. This change fixes this bug so that they will be considered equivalent. PiperOrigin-RevId: 702072912 --- pkg/BUILD.bazel | 1 + upb/message/BUILD | 4 ++-- upb/message/compare.c | 13 ++++++++----- upb/message/internal/extension.h | 17 +++++++++++++++++ upb/message/internal/message.h | 26 ++++++++++++++++++-------- upb/message/message.c | 9 +++++++-- upb/message/test.cc | 2 ++ 7 files changed, 55 insertions(+), 17 deletions(-) diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 792f11c155..49cfbd33fd 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -217,6 +217,7 @@ cc_dist_library( "//upb/message:compare", "//upb/message:copy", "//upb/text", + "//upb/text:debug", "//upb/util:def_to_proto", "//upb/util:required_fields", "//upb/wire:byte_size", diff --git a/upb/message/BUILD b/upb/message/BUILD index 0089596af5..de904bd61e 100644 --- a/upb/message/BUILD +++ b/upb/message/BUILD @@ -75,11 +75,9 @@ cc_library( deps = [ ":types", "//upb:base", - "//upb:eps_copy_input_stream", "//upb:mem", "//upb:mini_table", "//upb:port", - "//upb:wire_reader", "//upb/base:internal", "//upb/hash", "//upb/mini_table:internal", @@ -388,6 +386,8 @@ cc_test( "//upb:wire_reader", "//upb/test:fuzz_util", "//upb/test:test_messages_proto3_upb_proto", + "//upb/text:debug", + "@com_google_absl//absl/strings", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/upb/message/compare.c b/upb/message/compare.c index 6963ea5bdd..5266e0c830 100644 --- a/upb/message/compare.c +++ b/upb/message/compare.c @@ -133,20 +133,18 @@ static bool _upb_Message_ExtensionsAreEqual(const upb_Message* msg1, const upb_Message* msg2, const upb_MiniTable* m, int options) { - // Must have identical extension counts. - if (upb_Message_ExtensionCount(msg1) != upb_Message_ExtensionCount(msg2)) { - return false; - } - const upb_MiniTableExtension* e; upb_MessageValue val1; // Iterate over all extensions for msg1, and search msg2 for each extension. + size_t count1 = 0; size_t iter1 = kUpb_Message_ExtensionBegin; while (upb_Message_NextExtension(msg1, &e, &val1, &iter1)) { const upb_Extension* ext2 = UPB_PRIVATE(_upb_Message_Getext)(msg2, e); if (!ext2) return false; + count1++; + const upb_MessageValue val2 = ext2->data; const upb_MiniTableField* f = &e->UPB_PRIVATE(field); const upb_MiniTable* subm = upb_MiniTableField_IsSubMessage(f) @@ -170,6 +168,11 @@ static bool _upb_Message_ExtensionsAreEqual(const upb_Message* msg1, } if (!eq) return false; } + + // Must have identical extension counts (this catches the case where msg2 + // has extensions that msg1 doesn't). + if (count1 != upb_Message_ExtensionCount(msg2)) return false; + return true; } diff --git a/upb/message/internal/extension.h b/upb/message/internal/extension.h index 13f5b77d2f..0cfa1a9a62 100644 --- a/upb/message/internal/extension.h +++ b/upb/message/internal/extension.h @@ -11,8 +11,12 @@ #include #include "upb/mem/arena.h" +#include "upb/message/internal/array.h" +#include "upb/message/internal/map.h" +#include "upb/message/internal/types.h" #include "upb/message/value.h" #include "upb/mini_table/extension.h" +#include "upb/mini_table/internal/field.h" // Must be last. #include "upb/port/def.inc" @@ -51,6 +55,19 @@ const upb_Extension* UPB_PRIVATE(_upb_Message_Getexts)( const upb_Extension* UPB_PRIVATE(_upb_Message_Getext)( const struct upb_Message* msg, const upb_MiniTableExtension* ext); +UPB_INLINE bool UPB_PRIVATE(_upb_Extension_IsEmpty)(const upb_Extension* ext) { + switch ( + UPB_PRIVATE(_upb_MiniTableField_Mode)(&ext->ext->UPB_PRIVATE(field))) { + case kUpb_FieldMode_Scalar: + return false; + case kUpb_FieldMode_Array: + return upb_Array_Size(ext->data.array_val) == 0; + case kUpb_FieldMode_Map: + return _upb_Map_Size(ext->data.map_val) == 0; + } + UPB_UNREACHABLE(); +} + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/upb/message/internal/message.h b/upb/message/internal/message.h index 67fca81808..681c30fa5c 100644 --- a/upb/message/internal/message.h +++ b/upb/message/internal/message.h @@ -141,12 +141,17 @@ UPB_INLINE bool upb_Message_NextExtension(const struct upb_Message* msg, size_t count; const upb_Extension* exts = UPB_PRIVATE(_upb_Message_Getexts)(msg, &count); size_t i = *iter; - if (i < count) { + while (i++ < count) { // Extensions are stored in reverse wire order, so to iterate in wire order, // we need to iterate backwards. - *out_e = exts[count - 1 - i].ext; - *out_v = exts[count - 1 - i].data; - *iter = i + 1; + const upb_Extension* ext = &exts[count - i]; + + // Empty repeated fields or maps semantically don't exist. + if (UPB_PRIVATE(_upb_Extension_IsEmpty)(ext)) continue; + + *out_e = ext->ext; + *out_v = ext->data; + *iter = i; return true; } @@ -159,11 +164,16 @@ UPB_INLINE bool UPB_PRIVATE(_upb_Message_NextExtensionReverse)( size_t count; const upb_Extension* exts = UPB_PRIVATE(_upb_Message_Getexts)(msg, &count); size_t i = *iter; - if (i < count) { + while (i++ < count) { // Extensions are stored in reverse wire order - *out_e = exts[i].ext; - *out_v = exts[i].data; - *iter = i + 1; + const upb_Extension* ext = &exts[i - 1]; + + // Empty repeated fields or maps semantically don't exist. + if (UPB_PRIVATE(_upb_Extension_IsEmpty)(ext)) continue; + + *out_e = ext->ext; + *out_v = ext->data; + *iter = i; return true; } diff --git a/upb/message/message.c b/upb/message/message.c index 14b7eeb5ba..117b014fe4 100644 --- a/upb/message/message.c +++ b/upb/message/message.c @@ -117,8 +117,13 @@ bool upb_Message_DeleteUnknown(upb_Message* msg, upb_StringView* data, } size_t upb_Message_ExtensionCount(const upb_Message* msg) { - size_t count; - UPB_PRIVATE(_upb_Message_Getexts)(msg, &count); + const upb_MiniTableExtension* e; + upb_MessageValue val; + size_t iter = kUpb_Message_ExtensionBegin; + size_t count = 0; + while (upb_Message_NextExtension(msg, &e, &val, &iter)) { + count++; + } return count; } diff --git a/upb/message/test.cc b/upb/message/test.cc index 041a82d374..ed46c8c56e 100644 --- a/upb/message/test.cc +++ b/upb/message/test.cc @@ -15,6 +15,7 @@ #include #include +#include "absl/strings/escaping.h" #include "google/protobuf/test_messages_proto3.upb.h" #include "upb/base/status.hpp" #include "upb/base/string_view.h" @@ -38,6 +39,7 @@ #include "upb/reflection/def.hpp" #include "upb/reflection/message.h" #include "upb/test/fuzz_util.h" +#include "upb/text/debug_string.h" #include "upb/wire/decode.h" #include "upb/wire/encode.h" #include "upb/wire/eps_copy_input_stream.h"