524193 - IPDL: Fine-grained shmem access passing

Status: RESOLVED WONTFIX

(Core :: IPC, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v1

Implements the proposal on wmo, with some minor modifications.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 409028 [details] [diff] [review]
v1

For the IPDL compiler stuff

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v1, un-bitrotted

For Shmem stuff.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 412067 [details] [diff] [review]
v1, un-bitrotted

For IPDL stuff.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: updated

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

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: updated again

Oops, last version had some unused code.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 8

[Joe Drew (not getting mail)]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Just wondering what is happening with this bug.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.