From 610404dcde46009caadcabec774f7978888feddc Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Wed, 27 Jul 2022 19:13:02 +0000 Subject: [PATCH] Bug 1779829: Replace `Shmem::PrivateIPDLCaller` with `friend` specifiers. r=nika Instead of using the public member type `PrivateIPDLCaller` to restrict access to certain `Shmem` member functions, make them `private`, and designated `IProtocol` and `ITopLevelProtocol` as friends of `Shmem`. Differential Revision: https://phabricator.services.mozilla.com/D151952 --- ipc/glue/ProtocolUtils.cpp | 30 ++++++++--------- ipc/glue/Shmem.cpp | 36 ++++++++------------ ipc/glue/Shmem.h | 53 +++++++++++++++++------------- tools/fuzzing/ipc/ProtocolFuzzer.h | 7 ++-- 4 files changed, 60 insertions(+), 66 deletions(-) diff --git a/ipc/glue/ProtocolUtils.cpp b/ipc/glue/ProtocolUtils.cpp index 77ce6045b621..24ca77996c84 100644 --- a/ipc/glue/ProtocolUtils.cpp +++ b/ipc/glue/ProtocolUtils.cpp @@ -415,7 +415,7 @@ bool IProtocol::AllocShmem(size_t aSize, Shmem* aOutMem) { return false; } - *aOutMem = Shmem(Shmem::PrivateIPDLCaller(), rawmem, id); + *aOutMem = Shmem(rawmem, id); return true; } @@ -432,7 +432,7 @@ bool IProtocol::AllocUnsafeShmem(size_t aSize, Shmem* aOutMem) { return false; } - *aOutMem = Shmem(Shmem::PrivateIPDLCaller(), rawmem, id); + *aOutMem = Shmem(rawmem, id); return true; } @@ -448,7 +448,7 @@ bool IProtocol::DeallocShmem(Shmem& aMem) { return false; } #endif // DEBUG - aMem.forget(Shmem::PrivateIPDLCaller()); + aMem.forget(); return ok; } @@ -673,22 +673,20 @@ void IToplevelProtocol::Unregister(int32_t aId) { Shmem::SharedMemory* IToplevelProtocol::CreateSharedMemory(size_t aSize, bool aUnsafe, Shmem::id_t* aId) { - RefPtr segment( - Shmem::Alloc(Shmem::PrivateIPDLCaller(), aSize, aUnsafe)); + RefPtr segment(Shmem::Alloc(aSize, aUnsafe)); if (!segment) { return nullptr; } int32_t id = NextId(); - Shmem shmem(Shmem::PrivateIPDLCaller(), segment.get(), id); + Shmem shmem(segment.get(), id); - UniquePtr descriptor = - shmem.MkCreatedMessage(Shmem::PrivateIPDLCaller(), MSG_ROUTING_CONTROL); + UniquePtr descriptor = shmem.MkCreatedMessage(MSG_ROUTING_CONTROL); if (!descriptor) { return nullptr; } Unused << GetIPCChannel()->Send(std::move(descriptor)); - *aId = shmem.Id(Shmem::PrivateIPDLCaller()); + *aId = shmem.Id(); Shmem::SharedMemory* rawSegment = segment.get(); MOZ_ASSERT(!mShmemMap.Contains(*aId), "Don't insert with an existing ID"); mShmemMap.InsertOrUpdate(*aId, segment.forget().take()); @@ -709,19 +707,18 @@ bool IToplevelProtocol::IsTrackingSharedMemory(Shmem::SharedMemory* segment) { } bool IToplevelProtocol::DestroySharedMemory(Shmem& shmem) { - Shmem::id_t aId = shmem.Id(Shmem::PrivateIPDLCaller()); + Shmem::id_t aId = shmem.Id(); Shmem::SharedMemory* segment = LookupSharedMemory(aId); if (!segment) { return false; } - UniquePtr descriptor = - shmem.MkDestroyedMessage(Shmem::PrivateIPDLCaller(), MSG_ROUTING_CONTROL); + UniquePtr descriptor = shmem.MkDestroyedMessage(MSG_ROUTING_CONTROL); MOZ_ASSERT(mShmemMap.Contains(aId), "Attempting to remove an ID not in the shmem map"); mShmemMap.Remove(aId); - Shmem::Dealloc(Shmem::PrivateIPDLCaller(), segment); + Shmem::Dealloc(segment); MessageChannel* channel = GetIPCChannel(); if (!channel->CanSend()) { @@ -733,15 +730,14 @@ bool IToplevelProtocol::DestroySharedMemory(Shmem& shmem) { void IToplevelProtocol::DeallocShmems() { for (const auto& shmem : mShmemMap.Values()) { - Shmem::Dealloc(Shmem::PrivateIPDLCaller(), shmem); + Shmem::Dealloc(shmem); } mShmemMap.Clear(); } bool IToplevelProtocol::ShmemCreated(const Message& aMsg) { Shmem::id_t id; - RefPtr rawmem( - Shmem::OpenExisting(Shmem::PrivateIPDLCaller(), aMsg, &id, true)); + RefPtr rawmem(Shmem::OpenExisting(aMsg, &id, true)); if (!rawmem) { return false; } @@ -763,7 +759,7 @@ bool IToplevelProtocol::ShmemDestroyed(const Message& aMsg) { MOZ_ASSERT(mShmemMap.Contains(id), "Attempting to remove an ID not in the shmem map"); mShmemMap.Remove(id); - Shmem::Dealloc(Shmem::PrivateIPDLCaller(), rawmem); + Shmem::Dealloc(rawmem); } return true; } diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp index 7a27c0c724b5..bc0294f83e59 100644 --- a/ipc/glue/Shmem.cpp +++ b/ipc/glue/Shmem.cpp @@ -210,7 +210,7 @@ static void Unprotect(SharedMemory* aSegment) { // to touch the segment, it dies with SIGSEGV. // -Shmem::Shmem(PrivateIPDLCaller, SharedMemory* aSegment, id_t aId) +Shmem::Shmem(SharedMemory* aSegment, id_t aId) : mSegment(aSegment), mData(nullptr), mSize(0) { MOZ_ASSERT(mSegment, "null segment"); MOZ_ASSERT(aId != 0, "invalid ID"); @@ -259,7 +259,7 @@ void Shmem::AssertInvariants() const { Unused << checkMappingBack; } -void Shmem::RevokeRights(PrivateIPDLCaller) { +void Shmem::RevokeRights() { AssertInvariants(); size_t pageSize = SharedMemory::SystemPageSize(); @@ -276,8 +276,7 @@ void Shmem::RevokeRights(PrivateIPDLCaller) { } // static -already_AddRefed Shmem::Alloc(PrivateIPDLCaller, - size_t aNBytes, bool aUnsafe, +already_AddRefed Shmem::Alloc(size_t aNBytes, bool aUnsafe, bool aProtect) { NS_ASSERTION(aNBytes <= UINT32_MAX, "Will truncate shmem segment size!"); MOZ_ASSERT(!aProtect || !aUnsafe, "protect => !unsafe"); @@ -312,8 +311,7 @@ already_AddRefed Shmem::Alloc(PrivateIPDLCaller, // static already_AddRefed Shmem::OpenExisting( - PrivateIPDLCaller, const IPC::Message& aDescriptor, id_t* aId, - bool aProtect) { + const IPC::Message& aDescriptor, id_t* aId, bool aProtect) { size_t size; size_t pageSize = SharedMemory::SystemPageSize(); // |2*pageSize| is for the front and back sentinels @@ -345,7 +343,7 @@ already_AddRefed Shmem::OpenExisting( } // static -void Shmem::Dealloc(PrivateIPDLCaller, SharedMemory* aSegment) { +void Shmem::Dealloc(SharedMemory* aSegment) { if (!aSegment) return; size_t pageSize = SharedMemory::SystemPageSize(); @@ -365,7 +363,7 @@ void Shmem::Dealloc(PrivateIPDLCaller, SharedMemory* aSegment) { #else // !defined(DEBUG) -Shmem::Shmem(PrivateIPDLCaller, SharedMemory* aSegment, id_t aId) +Shmem::Shmem(SharedMemory* aSegment, id_t aId) : mSegment(aSegment), mData(aSegment->memory()), mSize(0), mId(aId) { mSize = static_cast(*PtrToSize(mSegment)); MOZ_RELEASE_ASSERT(mSegment->Size() - sizeof(uint32_t) >= mSize, @@ -373,8 +371,7 @@ Shmem::Shmem(PrivateIPDLCaller, SharedMemory* aSegment, id_t aId) } // static -already_AddRefed Shmem::Alloc(PrivateIPDLCaller, - size_t aNBytes, +already_AddRefed Shmem::Alloc(size_t aNBytes, bool /*unused*/, bool /*unused*/) { RefPtr segment = CreateSegment(aNBytes, sizeof(uint32_t)); @@ -389,8 +386,7 @@ already_AddRefed Shmem::Alloc(PrivateIPDLCaller, // static already_AddRefed Shmem::OpenExisting( - PrivateIPDLCaller, const IPC::Message& aDescriptor, id_t* aId, - bool /*unused*/) { + const IPC::Message& aDescriptor, id_t* aId, bool /*unused*/) { size_t size; RefPtr segment = ReadSegment(aDescriptor, aId, &size, sizeof(uint32_t)); @@ -407,14 +403,11 @@ already_AddRefed Shmem::OpenExisting( } // static -void Shmem::Dealloc(PrivateIPDLCaller, SharedMemory* aSegment) { - DestroySegment(aSegment); -} +void Shmem::Dealloc(SharedMemory* aSegment) { DestroySegment(aSegment); } #endif // if defined(DEBUG) -UniquePtr Shmem::MkCreatedMessage(PrivateIPDLCaller, - int32_t routingId) { +UniquePtr Shmem::MkCreatedMessage(int32_t routingId) { AssertInvariants(); auto msg = MakeUnique(routingId, mId, mSize); @@ -427,8 +420,7 @@ UniquePtr Shmem::MkCreatedMessage(PrivateIPDLCaller, return msg; } -UniquePtr Shmem::MkDestroyedMessage(PrivateIPDLCaller, - int32_t routingId) { +UniquePtr Shmem::MkDestroyedMessage(int32_t routingId) { AssertInvariants(); return MakeUnique(routingId, mId); } @@ -437,8 +429,8 @@ void IPDLParamTraits::Write(IPC::MessageWriter* aWriter, IProtocol* aActor, Shmem&& aParam) { WriteIPDLParam(aWriter, aActor, aParam.mId); - aParam.RevokeRights(Shmem::PrivateIPDLCaller()); - aParam.forget(Shmem::PrivateIPDLCaller()); + aParam.RevokeRights(); + aParam.forget(); } bool IPDLParamTraits::Read(IPC::MessageReader* aReader, @@ -450,7 +442,7 @@ bool IPDLParamTraits::Read(IPC::MessageReader* aReader, Shmem::SharedMemory* rawmem = aActor->LookupSharedMemory(id); if (rawmem) { - *aResult = Shmem(Shmem::PrivateIPDLCaller(), rawmem, id); + *aResult = Shmem(rawmem, id); return true; } *aResult = Shmem(); diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h index 341c53e84218..abeac77f86eb 100644 --- a/ipc/glue/Shmem.h +++ b/ipc/glue/Shmem.h @@ -60,36 +60,44 @@ class ShadowLayerForwarder; namespace ipc { +class IProtocol; +class IToplevelProtocol; + +#ifdef FUZZING +class ProtocolFuzzerHelper; +#endif + template struct IPDLParamTraits; class Shmem final { friend struct IPDLParamTraits; + friend class mozilla::ipc::IProtocol; + friend class mozilla::ipc::IToplevelProtocol; #ifdef DEBUG // For ShadowLayerForwarder::CheckSurfaceDescriptor friend class mozilla::layers::ShadowLayerForwarder; #endif +#ifdef FUZZING + friend class ProtocolFuzzerHelper; + template + friend void FuzzProtocol(T*, const uint8_t*, size_t, + const nsTArray&); +#endif public: typedef int32_t id_t; // Low-level wrapper around platform shmem primitives. typedef mozilla::ipc::SharedMemory SharedMemory; - // Shmem objects should only be constructed directly from SharedMemory - // objects by the Shmem implementation itself, or by a select few functions - // in ProtocolUtils.{h,cpp}. You should not need to add new instances of - // this token. - struct PrivateIPDLCaller {}; Shmem() : mSegment(nullptr), mData(nullptr), mSize(0), mId(0) {} Shmem(const Shmem& aOther) = default; - Shmem(PrivateIPDLCaller, SharedMemory* aSegment, id_t aId); - ~Shmem() { // Shmem only holds a "weak ref" to the actual segment, which is // owned by IPDL. So there's nothing interesting to be done here - forget(PrivateIPDLCaller()); + forget(); } Shmem& operator=(const Shmem& aRhs) = default; @@ -130,26 +138,29 @@ class Shmem final { return {get(), Size()}; } + private: // These shouldn't be used directly, use the IPDL interface instead. - id_t Id(PrivateIPDLCaller) const { return mId; } - SharedMemory* Segment(PrivateIPDLCaller) const { return mSegment; } + Shmem(SharedMemory* aSegment, id_t aId); + + id_t Id() const { return mId; } + + SharedMemory* Segment() const { return mSegment; } #ifndef DEBUG - void RevokeRights(PrivateIPDLCaller) {} + void RevokeRights() {} #else - void RevokeRights(PrivateIPDLCaller); + void RevokeRights(); #endif - void forget(PrivateIPDLCaller) { + void forget() { mSegment = nullptr; mData = nullptr; mSize = 0; mId = 0; } - static already_AddRefed Alloc(PrivateIPDLCaller, - size_t aNBytes, + static already_AddRefed Alloc(size_t aNBytes, bool aUnsafe, bool aProtect = false); @@ -157,27 +168,23 @@ class Shmem final { // contains enough information for the other process to map this segment in // OpenExisting() below. Return a new message if successful (owned by the // caller), nullptr if not. - UniquePtr MkCreatedMessage(PrivateIPDLCaller, - int32_t routingId); + UniquePtr MkCreatedMessage(int32_t routingId); // Stop sharing this with another process. Return an IPC message that // contains enough information for the other process to unmap this // segment. Return a new message if successful (owned by the // caller), nullptr if not. - UniquePtr MkDestroyedMessage(PrivateIPDLCaller, - int32_t routingId); + UniquePtr MkDestroyedMessage(int32_t routingId); // Return a SharedMemory instance in this process using the descriptor shared // to us by the process that created the underlying OS shmem resource. The // contents of the descriptor depend on the type of SharedMemory that was // passed to us. static already_AddRefed OpenExisting( - PrivateIPDLCaller, const IPC::Message& aDescriptor, id_t* aId, - bool aProtect = false); + const IPC::Message& aDescriptor, id_t* aId, bool aProtect = false); - static void Dealloc(PrivateIPDLCaller, SharedMemory* aSegment); + static void Dealloc(SharedMemory* aSegment); - private: template void AssertAligned() const { if (0 != (mSize % sizeof(T))) MOZ_CRASH("shmem is not T-aligned"); diff --git a/tools/fuzzing/ipc/ProtocolFuzzer.h b/tools/fuzzing/ipc/ProtocolFuzzer.h index f6782a8f2d90..b5708a258d09 100644 --- a/tools/fuzzing/ipc/ProtocolFuzzer.h +++ b/tools/fuzzing/ipc/ProtocolFuzzer.h @@ -72,13 +72,12 @@ void FuzzProtocol(T* aProtocol, const uint8_t* aData, size_t aSize, if (shmem_size > aSize) { break; } - RefPtr segment( - Shmem::Alloc(Shmem::PrivateIPDLCaller(), shmem_size, false)); + RefPtr segment(Shmem::Alloc(shmem_size, false)); if (!segment) { break; } - Shmem shmem(Shmem::PrivateIPDLCaller(), segment.get(), i + 1); + Shmem shmem(segment.get(), i + 1); memcpy(shmem.get(), aData, shmem_size); ProtocolFuzzerHelper::AddShmemToProtocol( aProtocol, segment.forget().take(), i + 1); @@ -99,7 +98,7 @@ void FuzzProtocol(T* aProtocol, const uint8_t* aData, size_t aSize, } for (uint32_t i = 0; i < num_shmems; i++) { Shmem::SharedMemory* segment = aProtocol->LookupSharedMemory(i + 1); - Shmem::Dealloc(Shmem::PrivateIPDLCaller(), segment); + Shmem::Dealloc(segment); ProtocolFuzzerHelper::RemoveShmemFromProtocol(aProtocol, i + 1); } }