From c29dfb5e2db1703bb14483062da64ec05649b57f Mon Sep 17 00:00:00 2001 From: Hong Shin Date: Wed, 18 Sep 2024 10:18:10 -0700 Subject: [PATCH] Disallow toplevel hpb::Ptr access to it's internal arena ``` hpb::Arena arena; auto foo = hpb::CreateMessage(arena); foo.GetInternalArena(); // this call will now be impossible ``` Before this CL, any Proxy/Ptr could get its internal arena. This unnecessarily leaks upb internals when they should be interacted with via hpb::interop::upb::*. PiperOrigin-RevId: 676038573 --- hpb/backend/upb/interop.h | 4 ++-- hpb/hpb.h | 26 +++++++++++++------------- hpb/internal/internal.h | 4 ++++ hpb_generator/gen_messages.cc | 9 +++++---- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/hpb/backend/upb/interop.h b/hpb/backend/upb/interop.h index fee89416a9..d0a7ebbe91 100644 --- a/hpb/backend/upb/interop.h +++ b/hpb/backend/upb/interop.h @@ -51,12 +51,12 @@ auto* GetMessage(T&& message) { template upb_Arena* GetArena(Ptr message) { - return static_cast(message->GetInternalArena()); + return hpb::internal::PrivateAccess::GetInternalArena(message); } template upb_Arena* GetArena(T* message) { - return static_cast(message->GetInternalArena()); + return hpb::internal::PrivateAccess::GetInternalArena(message); } /** diff --git a/hpb/hpb.h b/hpb/hpb.h index 8186142bf7..da47212483 100644 --- a/hpb/hpb.h +++ b/hpb/hpb.h @@ -141,7 +141,7 @@ absl::Status SetExtension( const ::hpb::internal::ExtensionIdentifier& id, const Extension& value) { static_assert(!std::is_const_v); - auto* message_arena = static_cast(message->GetInternalArena()); + auto* message_arena = hpb::interop::upb::GetArena(message); return ::hpb::internal::SetExtension(hpb::interop::upb::GetMessage(message), message_arena, id.mini_table_ext(), hpb::interop::upb::GetMessage(&value)); @@ -155,7 +155,7 @@ absl::Status SetExtension( const ::hpb::internal::ExtensionIdentifier& id, Ptr value) { static_assert(!std::is_const_v); - auto* message_arena = static_cast(message->GetInternalArena()); + auto* message_arena = hpb::interop::upb::GetArena(message); return ::hpb::internal::SetExtension(hpb::interop::upb::GetMessage(message), message_arena, id.mini_table_ext(), hpb::interop::upb::GetMessage(value)); @@ -170,8 +170,8 @@ absl::Status SetExtension( Extension&& value) { Extension ext = std::move(value); static_assert(!std::is_const_v); - auto* message_arena = static_cast(message->GetInternalArena()); - auto* extension_arena = static_cast(ext.GetInternalArena()); + auto* message_arena = hpb::interop::upb::GetArena(message); + auto* extension_arena = hpb::interop::upb::GetArena(&ext); return ::hpb::internal::MoveExtension(hpb::interop::upb::GetMessage(message), message_arena, id.mini_table_ext(), hpb::interop::upb::GetMessage(&ext), @@ -251,10 +251,10 @@ typename T::Proxy CloneMessage(Ptr message, upb_Arena* arena) { template void DeepCopy(Ptr source_message, Ptr target_message) { static_assert(!std::is_const_v); - ::hpb::internal::DeepCopy( - hpb::interop::upb::GetMessage(target_message), - hpb::interop::upb::GetMessage(source_message), T::minitable(), - static_cast(target_message->GetInternalArena())); + ::hpb::internal::DeepCopy(hpb::interop::upb::GetMessage(target_message), + hpb::interop::upb::GetMessage(source_message), + T::minitable(), + hpb::interop::upb::GetArena(target_message)); } template @@ -285,7 +285,7 @@ ABSL_MUST_USE_RESULT bool Parse(Ptr message, absl::string_view bytes) { static_assert(!std::is_const_v); upb_Message_Clear(hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message)); - auto* arena = static_cast(message->GetInternalArena()); + auto* arena = hpb::interop::upb::GetArena(message); return upb_Decode(bytes.data(), bytes.size(), hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message), @@ -300,7 +300,7 @@ ABSL_MUST_USE_RESULT bool Parse( static_assert(!std::is_const_v); upb_Message_Clear(hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message)); - auto* arena = static_cast(message->GetInternalArena()); + auto* arena = hpb::interop::upb::GetArena(message); return upb_Decode(bytes.data(), bytes.size(), hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message), @@ -322,7 +322,7 @@ ABSL_MUST_USE_RESULT bool Parse(T* message, absl::string_view bytes) { static_assert(!std::is_const_v); upb_Message_Clear(hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message)); - auto* arena = static_cast(message->GetInternalArena()); + auto* arena = hpb::interop::upb::GetArena(message); return upb_Decode(bytes.data(), bytes.size(), hpb::interop::upb::GetMessage(message), ::hpb::interop::upb::GetMiniTable(message), @@ -333,7 +333,7 @@ ABSL_MUST_USE_RESULT bool Parse(T* message, absl::string_view bytes) { template absl::StatusOr Parse(absl::string_view bytes, int options = 0) { T message; - auto* arena = static_cast(message.GetInternalArena()); + auto* arena = hpb::interop::upb::GetArena(&message); upb_DecodeStatus status = upb_Decode(bytes.data(), bytes.size(), message.msg(), ::hpb::interop::upb::GetMiniTable(&message), @@ -349,7 +349,7 @@ absl::StatusOr Parse(absl::string_view bytes, const ::hpb::ExtensionRegistry& extension_registry, int options = 0) { T message; - auto* arena = static_cast(message.GetInternalArena()); + auto* arena = hpb::interop::upb::GetArena(&message); upb_DecodeStatus status = upb_Decode(bytes.data(), bytes.size(), message.msg(), ::hpb::interop::upb::GetMiniTable(&message), diff --git a/hpb/internal/internal.h b/hpb/internal/internal.h index 319fa87dea..420071c4a8 100644 --- a/hpb/internal/internal.h +++ b/hpb/internal/internal.h @@ -14,6 +14,10 @@ struct PrivateAccess { return message->msg(); } template + static auto* GetInternalArena(T&& message) { + return message->arena(); + } + template static auto Proxy(upb_Message* p, upb_Arena* arena) { return typename T::Proxy(p, arena); } diff --git a/hpb_generator/gen_messages.cc b/hpb_generator/gen_messages.cc index 1a367449ef..ca6932a80e 100644 --- a/hpb_generator/gen_messages.cc +++ b/hpb_generator/gen_messages.cc @@ -100,7 +100,6 @@ void WriteModelAccessDeclaration(const protobuf::Descriptor* descriptor, : msg_(const_cast<$1*>(msg)), arena_(arena) { assert(arena != nullptr); } // NOLINT - void* GetInternalArena() const { return arena_; } )cc", ClassName(descriptor), MessageName(descriptor)); WriteFieldAccessorsInHeader(descriptor, output); @@ -225,7 +224,6 @@ void WriteModelPublicDeclaration( output( R"cc( static const upb_MiniTable* minitable(); - using $0Access::GetInternalArena; )cc", ClassName(descriptor)); output("\n"); @@ -236,6 +234,8 @@ void WriteModelPublicDeclaration( const upb_Message* msg() const { return UPB_UPCAST(msg_); } upb_Message* msg() { return UPB_UPCAST(msg_); } + upb_Arena* arena() const { return arena_; } + $0(upb_Message* msg, upb_Arena* arena) : $0Access() { msg_ = ($1*)msg; arena_ = owned_arena_.ptr(); @@ -282,7 +282,6 @@ void WriteModelProxyDeclaration(const protobuf::Descriptor* descriptor, arena_ = m.arena_; return *this; } - using $0Access::GetInternalArena; )cc", ClassName(descriptor)); @@ -295,6 +294,8 @@ void WriteModelProxyDeclaration(const protobuf::Descriptor* descriptor, private: upb_Message* msg() const { return UPB_UPCAST(msg_); } + upb_Arena* arena() const { return arena_; } + $0Proxy(upb_Message* msg, upb_Arena* arena) : internal::$0Access(($1*)msg, arena) {} friend $0::Proxy(::hpb::CreateMessage<$0>(::hpb::Arena& arena)); @@ -334,7 +335,6 @@ void WriteModelCProxyDeclaration(const protobuf::Descriptor* descriptor, $0CProxy(const $0* m) : internal::$0Access(m->msg_, hpb::interop::upb::GetArena(m)) {} $0CProxy($0Proxy m); - using $0Access::GetInternalArena; )cc", ClassName(descriptor), MessageName(descriptor)); @@ -347,6 +347,7 @@ void WriteModelCProxyDeclaration(const protobuf::Descriptor* descriptor, private: using AsNonConst = $0Proxy; const upb_Message* msg() const { return UPB_UPCAST(msg_); } + upb_Arena* arena() const { return arena_; } $0CProxy(const upb_Message* msg, upb_Arena* arena) : internal::$0Access(($1*)msg, arena){};