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.

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.

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

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.

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.

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.

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.

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 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.

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

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.

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

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 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.

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.

(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 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,"?

Good call, this language is confusing.

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