The proposed feature is documented at https://wiki.mozilla.org/IPDL/Proposal:Shmem_access_control .

Implements the proposal on wmo, with some minor modifications.

Comment on attachment 409028 [details] [diff] [review] v1 For the IPDL compiler stuff

For Shmem stuff.

Comment on attachment 412067 [details] [diff] [review] v1, un-bitrotted For IPDL stuff.

Just a couple of trivial merge things, nothing requiring re-r?.

Oops, last version had some unused code.

Comment on attachment 415339 [details] [diff] [review] updated again The IPDL stuff looks ok to me. What's with the NS_DEBUG_ASSERT changes?

Comment on attachment 412067 [details] [diff] [review] v1, un-bitrotted General comment: need to s/uint32_t/PRUint32/ throughout. >diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp >+void >+TransferRight(uint32_t aRight, >+ bool A_adding, bool A_removing, >+ bool B_adding, bool B_removing, >+ uint32_t& A_held, uint32_t& A_reserved) IMO, TransferRight is not the right name for this function, because it just updates A's rights. TransferRight should update both A and B's rights - UpdateRight would be better. Also, it should take as a parameter a single Rights enum instead of an integer, because we're transferring a single right. Finally, it really feels like this function has too many parameters for its usecase. A better signature would involve a tri-state, I think: A wants, A doesn't want, B wants. This could be expanded to four values if we wanted to update both A's and B's held and reserved values at the same time, which I'm in favour of. >+ShmemPairRightsTracker::TransferRights(RightsQuad aTransfer) Mostly skipping a review of TransferRights(), because of the changes that the above comments will entail. But one nit: >+ NS_ABORT_IF_FALSE(!meAddingRead >+ || (MeReservesRead() || (otherRights & RightsRead)), >+ "insufficient permission to add Read"); || and && go on the preceding line. (repeated a couple of times) > Shmem::Shmem(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead, > SharedMemory* aSegment, id_t aId) : >- mSegment(aSegment), >+ mSegment(0), nsnull > mData(0), >- mSize(0) >+ mSize(0), >+ mId(0) > { >- NS_ABORT_IF_FALSE(mSegment, "NULL segment"); >+ Init(aSegment, aId); >+} >+ >+Shmem::Shmem(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead, >+ const Shmem& aFrom, >+ int aMyAddedRights, int aMyRevokedRights, >+ int aOtherAddedRights, int aOtherRevokedRights) : >+ // we don't "map in" the shmem ... this instance is only being >+ // created for the purposed of IPC purpose_s_ >+ mSegment(0), nsnull >+ mData(0), nsnull >+ mSize(0), >+ mId(aFrom.mId), >+ mChmod(aMyAddedRights, aMyRevokedRights, >+ aOtherAddedRights, aOtherRevokedRights) >+{ >+ aFrom.AssertInvariants(); >+ >+ SharedMemory* segment = aFrom.mSegment; >+ segment->ChangeRights( >+ IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead(), mChmod); >+ Protect(segment); >+} >+ >+Shmem::Shmem(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead, >+ SharedMemory* aSegment, const Shmem& aInfo) : >+ mSegment(0), >+ mData(0), >+ mSize(0), >+ mId(0) >+{ >+ // TODO check aInfo.chmod against ... shmem segment Header? have to >+ // watch out for race conditions >+ aSegment->ChangeRights( >+ IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead(), >+ aInfo.mChmod); >+ Init(aSegment, aInfo.mId); >+} >+ >+void >+Shmem::Init(SharedMemory* aSegment, id_t aId) >+{ >+ NS_ABORT_IF_FALSE(aSegment, "NULL segment"); > NS_ABORT_IF_FALSE(aId != 0, "invalid ID"); > >- Unprotect(mSegment); >- >+ Unprotect(aSegment); > char* frontSentinel; > char* data; > char* backSentinel; > GetSections(aSegment, &frontSentinel, &data, &backSentinel); > >- // do a quick validity check to avoid weird-looking crashes in libc >- char check = *frontSentinel; >- (void)check; >- Why remove this? > // transition into the "mapped" state by protecting the front and > // back sentinels (which guard against buffer under/overflows) >- mSegment->Protect(frontSentinel, pageSize, RightsNone); >- mSegment->Protect(backSentinel, pageSize, RightsNone); >+ aSegment->Protect(frontSentinel, pageSize, RightsNone); >+ aSegment->Protect(backSentinel, pageSize, RightsNone); >+ Protect(aSegment); Add a comment saying that we want to set the data segment's rights to our current rights mask. > // don't set these until we know they're valid >+ mSegment = aSegment; > mData = data; > mId = aId; >-} >+ } stray space there >diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h >+// Sigh, what should this be named? It's just a way to squirrel away >+// 4 Rights bitfields packed into one uint32_t... >+struct RightsQuad Is there a good reason to have a separate RightsQuad and RightsTracker? It feels like it'd be clearer to just have a single Rights object that internalizes RightsQuad and RightsTracker's functionality. >+// Tracks the "held" and "reserved" rights that pair of actors hold >+// wrt to a Shmem::shmem_t. Shmem::SharedMemory, not shmem_t, right? >+// A client-usable reference to a shared-memory segment. These are >+// what IPDL hands out to actors; actors can't otherwise access the >+// underlying shmem_t. And here? > // These shouldn't be used directly, use the IPDL interface instead. >- id_t Id(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead) { >+ id_t Id(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead) const { > return mId; > } This should be in the patch for bug 523174. >diff --git a/xpcom/base/nsDebugImpl.cpp b/xpcom/base/nsDebugImpl.cpp >diff --git a/xpcom/build/nsXPCOM.h b/xpcom/build/nsXPCOM.h The changes for these files should probably be in a different bug, and reviewed by Benjamin Smedberg.

Just wondering what is happening with this bug.

It's not blocking fenntrolysis yet (last I heard!), so OOPP has taken precedence. I hope to get back to this within the next couple of weeks.

We've ended up not caring about this. The design is still on WMO in case we need to dust it off in the future.