523174 - Add a higher-level mozilla::ipc::shmem class

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

This C++ class should that "hide" the raw pointer to the shared mapping, offering C++ operators instead.  This class will implement single-process/thread ownership semantics.  This class also needs an IPC::ParamTraits<mozilla::ipc::shmem> specialization so that it can serialized and deserialized with IPDL.

Comment 1

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

After chatting with Joe and Jeff, it appears that this API wouldn't fly in gfx-land.  Instead, we'll enforce in-bounds access by mapping no-access pages in front of and behind the "real" segment, and enforce ownership semantics by unmapping or mprotect()ing the shmem segment when ownership changes.  These would both be DEBUG-only.  We're probably going to have give up thread-safety checks, although Jeff proposes catching SIGSEGV or using a valgrind tool to do so.

Comment 2

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

Attachment: WIP

Attaching WIP in case Joe and Jeff want to pre-review the API.

Comment 3

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

Attachment: v1

The long comment in ipc/glue/Shmem.cpp is the best reference for the protection scheme I implemented.  All protection checks go away in opt builds, though we may be able to introduce them back if they're not that expensive in practice.

Comment 4

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

Joe, Jeff: I should note that the placeholder code in ipc/glue/SharedMemory.h is POSIX-only atm, and not portable even there.  I just added portable SystemPageSize(), and Windows implementations of SystemPageSize() and Protect().  I'm testing the Windows impls right now.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

It's hard to review this api without it having any users. What are the usecases we're all trying to cover?

Those questions aside, it does look mostly sane.

Comment 6

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

I think it might be useful to look at the testcase (ipc/ipdl/test/cxx/TestShmem.cpp and PTestShmem.ipdl) in bug 523175 to see how everything fits together.  This is just one piece in that puzzle: mozilla::ipc::Shmem implements ownership semantics and buffer underflow/overflow checks on top of "bare" shmem (mozilla::ipc::SharedMemory).

I'm not 100% sure how to answer your question, so I'll track you down on IRC to get more info.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I basically meant that without knowing what we're all going to be using mozilla::ipc::shmem for makes it hard to know if mozilla::ipc::shmem is a good fit for those use cases.

Comment 8

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

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

This code doesn't quite provide all we need. See bug 524180 for the additions that will be made.

Comment 9

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

Attachment: v1, refreshed

Minor bitrot.  Just to warn, I'm going to change the DEBUG sentinel scheme slightly as part of bug 524193.

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 411219 [details] [diff] [review]
v1, refreshed


>+++ b/ipc/glue/SharedMemory.h

>+#if defined(OS_WIN)
>+#  include <windows.h>
>+#else

We should avoid including windows.h as hard as we can.

>+  void Protect(char* aAddr, size_t aSize, int aRights)
>+  {
>+    char* base = reinterpret_cast<char*>(memory());
>+    NS_ABORT_IF_FALSE((base <= aAddr) && (aAddr < (base + Size())),
>+                      "address out of bounds");

We probably want to pair this with some form of runtime detection too. (Both for the Windows and POSIX codepaths.)

>+++ b/ipc/glue/Shmem.cpp

>+    // transition into the "mapped" state
>+    mSegment->Protect(frontSentinel, pageSize, RightsNone);
>+    mSegment->Protect(backSentinel, pageSize, RightsNone);

Add a better comment about the fact that we don't want rights on the sentinel pages to ensure we catch accidental accesses.

>+Shmem::Show(shmem_t* aSegment)
>+Shmem::Hide(shmem_t* aSegment)

Not in love with Show/Hide as names; Allow/Deny, Access/Deny might be better.

>+Shmem::shmem_t*
>+Shmem::Alloc(size_t aNBytes, shmemhandle_t aHandle, bool aHide)
>+    if (!aHide) {
>+      // if we're not initially hiding this mapping, we assume that we
>+      // are the ones that created the segment and so need to
>+      // initialize it.  This is how the IPDL-generated code works.
>+      Header* header = reinterpret_cast<Header*>(frontSentinel);
>+      memcpy(header->mMagic, sMagic, sizeof(sMagic));
>+      header->mSize = aNBytes;

This is pretty ugly. :( I guess it's only in debug code, but it's hard to follow the logic. I also don't like a parameter whose meaning is very important, but which is unused in the release builds.

Overall, that just feels brittle.

Also, Alloc/Dealloc being "internal-only," maybe they should be named e.g. __internal__only_Alloc?

>+size_t
>+Shmem::PageAlignedSize(size_t aSize)
>+{
>+    size_t pageSize = shmem_t::SystemPageSize();
>+    size_t nPages = aSize / pageSize;
>+    return pageSize * (nPages + ((aSize % pageSize) ? 1 : 0));
>+}

size_t nPagesNeeded = int(ceil(double(aSize) / pageSize));
return pageSize * nPagesNeeded;

would be clearer.

>+++ b/ipc/glue/Shmem.h

>+  typedef int32 id_t;
>+  // Low-level wrapper around platform shmem primitives.
>+  typedef mozilla::ipc::SharedMemory shmem_t;
>+  typedef shmem_t::SharedMemoryHandle shmemhandle_t;

I found the typedef of mozilla::ipc::SharedMemory as shmem_t obscured what was being called where. IMO, maybe just "using mozilla::ipc::SharedMemory" and s/shmem_t/SharedMemory/ would have been enough.


>+  // Return a pointer to the user-visible data segment.
>+  template<typename T>
>+  T*
>+  get() const
>+  {
>+    AssertInvariants();
>+    return reinterpret_cast<T*>(mData);
>+  }

This should probably do the same NS_ABORT_IF_FALSE T-aligned trick that Size() does. Better yet would be a compile-time assertion; I don't know if XPCOM has those, but they're pretty easy - just define a template that only has a true specialization.

>+  // These shouldn't be used directly, use the IPDL interface instead.
>+  id_t __internal__ipdl__id() { return mId; }
>+  static shmem_t* Alloc(size_t aNBytes,
>+                        shmemhandle_t aHandle=shmem_t::NULLHandle(),
>+                        bool aHide=false);
>+  static void Dealloc(shmem_t* aSegment);

Maybe rename, as mentioned above?

>+  static void Log(const paramType& aParam, std::wstring* aLog)
>+  {
>+    aLog->append(L"(shmem segment");

There's some weirdness with brackets there.

Comment 11

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

Attachment: v2, comments addressed + wstring fix

There was an OPT-only crash happening because of a chromium shmem method taking a wstring argument.  I hate wstrings.

Comment 12

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

Joe, did you want to re-review the updated patch?  You r+'d the first one, but the changes between first and second were more than just picking nits.

If not, I'll go ahead and push.

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 411829 [details] [diff] [review]
v2, comments addressed + wstring fix

>diff --git a/ipc/glue/SharedMemory.h b/ipc/glue/SharedMemory.h

>+class SharedMemory : public base::SharedMemory

It makes me uneasy to be overriding methods in base::SharedMemory that aren't virtual. Would it be better for us to enhance base::SharedMemory to suit our needs? I worry about people passing these SharedMemorys to chromium code.

>diff --git a/ipc/glue/SharedMemory_posix.cc b/ipc/glue/SharedMemory_posix.cc

>+void
>+SharedMemory::SystemProtect(char* aAddr, size_t aSize, int aRights)
>+{
>+  // FIXME real impl

What's wrong with this implementation?

> diff --git a/ipc/glue/SharedMemory_windows.cc b/ipc/glue/SharedMemory_windows.cc

>+SharedMemory::Protect(char* aAddr, size_t aSize, int aRights)

SharedMemory::SystemProtect(), right?

>diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp

>+Shmem::SharedMemory*
>+Shmem::OpenExisting(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead,
>+                    SharedMemoryHandle aHandle,
>+                    size_t aNBytes,
>+                    bool aProtect)

Why would I have a SharedMemoryHandle instead of a SharedMemory*? 

>+Shmem::CreateSegment(size_t aNBytes, SharedMemoryHandle aHandle)

>+  if (SharedMemory::IsHandleValid(aHandle)) {
>+    segment = new SharedMemory(aHandle);
>+    }

      ^ fix indent

>+  else {
>+    segment = new SharedMemory();
>+    if (!segment->Create("", false, false, aNBytes))

Guh, I wish that Chromium had chosen to use a const char USE_UNIQUE_NAME = "" or something.

>+Shmem::DestroySegment(SharedMemory* aSegment)
>+{
>+  delete aSegment;
>+}

Would you add a comment saying that SharedMemory's destructor takes care of unmapping the segment?

>diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h

>+  ~Shmem()
>+  {
>+    // this class only holds a "weak ref" to the actual segment,
>+    // so there's nothing interesting to be done here
>+    forget();
>+  }

Who holds strong refs to the segment? And why does this class have a DestroySegment() if it's only holding weak refs?

>+  template<typename T>
>+  void AssertAligned() const
>+  {
>+    NS_ABORT_IF_FALSE(0 == (mSize % sizeof(T)),
>+                      "shmem is not T-aligned");
>+  }

This should probably be a non-debug runtime abort, because otherwise it will just magically work on x86 and crash hard on all other platforms.

>+template<>
>+struct ParamTraits<mozilla::ipc::Shmem>
>+{
>+  typedef mozilla::ipc::Shmem paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    WriteParam(aMsg, aParam.mId);
>+    paramType::Protect(aParam.mSegment);
>+    const_cast<paramType&>(aParam).forget();
>+  }

Whoa. I really don't like side-effects like this in Write(). IMO the ipdl compiler should be emitting code to automatically forget/create the segment as necessary, and Read and Write should be simply doing what their name says.

Comment 14

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

Looking back over this bug and patch, I realize one glaring omission is a comment explaining the entire SharedMemory/Shmem/IPDL scheme at a high level.  I believe Shmem.h is probably the best place for that.

(In reply to comment #13)
> (From update of attachment 411829 [details] [diff] [review])
> >diff --git a/ipc/glue/SharedMemory.h b/ipc/glue/SharedMemory.h
> 
> >+class SharedMemory : public base::SharedMemory
> 
> It makes me uneasy to be overriding methods in base::SharedMemory that aren't
> virtual. Would it be better for us to enhance base::SharedMemory to suit our
> needs? I worry about people passing these SharedMemorys to chromium code.
> 

This is temporary, done so that this bug wouldn't block on bug 523172.  Also FWIW shmem consumers probably won't use the SharedMemory class directly.

> >diff --git a/ipc/glue/SharedMemory_posix.cc b/ipc/glue/SharedMemory_posix.cc
> 
> >+void
> >+SharedMemory::SystemProtect(char* aAddr, size_t aSize, int aRights)
> >+{
> >+  // FIXME real impl
> 
> What's wrong with this implementation?
> 

This comment is actually a vestige of when this impl was POSIX only.  That said, this isn't the "real" impl of mozilla::ipc::Shmem implementation (see bug 523172), though this comment doesn't help anyone.

> > diff --git a/ipc/glue/SharedMemory_windows.cc b/ipc/glue/SharedMemory_windows.cc
> 
> >+SharedMemory::Protect(char* aAddr, size_t aSize, int aRights)
> 
> SharedMemory::SystemProtect(), right?
> 

Yes.  This and a couple of other minor bugs were fixed locally, after I tested on Windows.  I didn't re-post because the changes were trivial.

> >diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp
> 
> >+Shmem::SharedMemory*
> >+Shmem::OpenExisting(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead,
> >+                    SharedMemoryHandle aHandle,
> >+                    size_t aNBytes,
> >+                    bool aProtect)
> 
> Why would I have a SharedMemoryHandle instead of a SharedMemory*? 
> 

A "SharedMemoryHandle" is an fd or HANDLE that refers to a shmem resource that hasn't yet been opened or mapped.  IPDL code uses this constructor after receiving notification from the other process that a shmem segment has been created and shared.

> >+  else {
> >+    segment = new SharedMemory();
> >+    if (!segment->Create("", false, false, aNBytes))
> 
> Guh, I wish that Chromium had chosen to use a const char USE_UNIQUE_NAME = ""
> or something.
> 

The need for this will evaporate after bug 523172 lands, although we might as well keep these changes.

> >diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h
> 
> >+  ~Shmem()
> >+  {
> >+    // this class only holds a "weak ref" to the actual segment,
> >+    // so there's nothing interesting to be done here
> >+    forget();
> >+  }
> 
> Who holds strong refs to the segment? And why does this class have a
> DestroySegment() if it's only holding weak refs?
> 

The to-be-added comment describing the grand scheme will document this, but essentially

  * SharedMemory is the underlying OS shmem resource
  * IPDL manages SharedMemorys, and doesn't expose them to consumers
  * Shmem holds a weak ref to a SharedMemory.  Shmems are what IPDL hands out to consumers

> >+template<>
> >+struct ParamTraits<mozilla::ipc::Shmem>
> >+{
> >+  typedef mozilla::ipc::Shmem paramType;
> >+
> >+  static void Write(Message* aMsg, const paramType& aParam)
> >+  {
> >+    WriteParam(aMsg, aParam.mId);
> >+    paramType::Protect(aParam.mSegment);
> >+    const_cast<paramType&>(aParam).forget();
> >+  }
> 
> Whoa. I really don't like side-effects like this in Write(). IMO the ipdl
> compiler should be emitting code to automatically forget/create the segment as
> necessary, and Read and Write should be simply doing what their name says.

Fair.  FWIW my decision here was between this and creating another gnarly C++-public-but-private-to-IPDL-code method on Shmem.  I'll switch this to the latter, which in retrospect seems preferable, point taken.

Comment 15

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

(In reply to comment #13)
> >+  ~Shmem()
> >+  {
> >+    // this class only holds a "weak ref" to the actual segment,
> >+    // so there's nothing interesting to be done here
> >+    forget();
> >+  }
> 
> Who holds strong refs to the segment? And why does this class have a
> DestroySegment() if it's only holding weak refs?
> 

I only answered the first question ... |Shmem| offers an Alloc/OpenExisting/Dealloc interface to IPDL.  This interface is used rather than directly allocating/deallocating |SharedMemory|s because the Shmem code implements the protection scheme.  Internally to Shmem, those IPDL-interface functions are implemented using the private functions CreateSegment/DestroySegment.  DestroySegment only exists because CreateSegment does; DestroySegment doesn't do anything interesting.

Comment 16

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

Attachment: v3

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 415314 [details] [diff] [review]
v3


>diff --git a/ipc/glue/Shmem.h b/ipc/glue/Shmem.h

>+ *  (4b) Simulatenously with the child receving the "shmem-created"
>+ *  IPC message, IPDL-generated code in the parent returns a
>+ *  |mozilla::ipc::Shmem| back to the C++ caller of
>+ *  |parentActor->AllocShmem()|.

Is this true? If it sends an async message, wouldn't/shouldn't it be "After sending the "shmem-created IPC message,"?

Comment 18

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

Good call, this language is confusing.

Comment 19

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

Pushed http://hg.mozilla.org/projects/electrolysis/rev/5ffc5409203c