Bug 1386422: Clean up error handling and stream positioning in StreamHandlerFromOBJREF; r=jimm

MozReview-Commit-ID: HmbZeX76bkb

ipc/mscom/Interceptor.cpp

@@ -306,15 +306,18 @@ Interceptor::MarshalInterface(IStream* pStm, REFIID riid, void* pv,
     DWORD callerTid;
     if (::CoGetCallerTID(&callerTid) == S_FALSE && callerTid != chromeMainTid) {
       // The caller isn't our chrome process, so do not provide a handler.
-      // First, seek back to the stream position that we prevously saved.
-      seekTo.QuadPart = objrefPos.QuadPart;
-      hr = pStm->Seek(seekTo, STREAM_SEEK_SET, nullptr);
+
+      // First, save the current position that marks the current end of the
+      // OBJREF in the stream.
+      ULARGE_INTEGER endPos;
+      hr = pStm->Seek(seekTo, STREAM_SEEK_CUR, &endPos);
       if (FAILED(hr)) {
         return hr;
       }
 
       // Now strip out the handler.
-      if (!StripHandlerFromOBJREF(WrapNotNull(pStm))) {
+      if (!StripHandlerFromOBJREF(WrapNotNull(pStm), objrefPos.QuadPart,
+                                  endPos.QuadPart)) {
         return E_FAIL;
       }
 

ipc/mscom/Objref.cpp

@@ -140,15 +140,14 @@ namespace mozilla {
 namespace mscom {
 
 bool
-StripHandlerFromOBJREF(NotNull<IStream*> aStream)
+StripHandlerFromOBJREF(NotNull<IStream*> aStream, const uint64_t aStartPos,
+                       const uint64_t aEndPos)
 {
-  // Get current stream position
+  // Ensure that the current stream position is set to the beginning
   LARGE_INTEGER seekTo;
-  seekTo.QuadPart = 0;
+  seekTo.QuadPart = aStartPos;
 
-  ULARGE_INTEGER objrefPos;
-
-  HRESULT hr = aStream->Seek(seekTo, STREAM_SEEK_CUR, &objrefPos);
+  HRESULT hr = aStream->Seek(seekTo, STREAM_SEEK_SET, nullptr);
   if (FAILED(hr)) {
     return false;
   }
@@ -164,10 +163,14 @@ StripHandlerFromOBJREF(NotNull<IStream*> aStream)
 
   uint32_t type;
   hr = aStream->Read(&type, sizeof(type), &bytesRead);
-  if (FAILED(hr) || bytesRead != sizeof(type) ||
-      type != OBJREF_TYPE_HANDLER) {
-    // If we're not a handler then just return success
-    return true;
+  if (FAILED(hr) || bytesRead != sizeof(type)) {
+    return false;
+  }
+  if (type != OBJREF_TYPE_HANDLER) {
+    // If we're not a handler then just seek to the end of the OBJREF and return
+    // success; there is nothing left to do.
+    seekTo.QuadPart = aEndPos;
+    return SUCCEEDED(aStream->Seek(seekTo, STREAM_SEEK_SET, nullptr));
   }
 
   IID iid;
@@ -217,7 +220,7 @@ StripHandlerFromOBJREF(NotNull<IStream*> aStream)
   }
 
   // Signature doesn't change so we'll seek past that
-  seekTo.QuadPart = objrefPos.QuadPart + sizeof(signature);
+  seekTo.QuadPart = aStartPos + sizeof(signature);
   hr = aStream->Seek(seekTo, STREAM_SEEK_SET, nullptr);
   if (FAILED(hr)) {
     return false;
@@ -251,7 +254,9 @@ StripHandlerFromOBJREF(NotNull<IStream*> aStream)
     return false;
   }
 
-  return true;
+  // Back up to just before the zeros we just wrote
+  seekTo.QuadPart = -static_cast<int64_t>(sizeof(CLSID));
+  return SUCCEEDED(aStream->Seek(seekTo, STREAM_SEEK_CUR, nullptr));
 }
 
 } // namespace mscom

ipc/mscom/Objref.h

@@ -17,13 +17,17 @@ namespace mscom {
 /**
  * Given a buffer containing a serialized proxy to an interface with a handler,
  * this function strips out the handler and converts it to a standard one.
- * @param aStream IStream whose pointer is positioned at the beginning of the
- *                OBJREF to be stripped. There should be nothing else written
- *                to the stream past the current OBJREF.
+ * @param aStream IStream containing a serialized proxy.
+ *                There should be nothing else written to the stream past the
+ *                current OBJREF.
+ * @param aStart  Absolute position of the beginning of the OBJREF.
+ * @param aEnd    Absolute position of the end of the OBJREF.
  * @return true if the handler was successfully stripped, otherwise false.
  */
 bool
-StripHandlerFromOBJREF(NotNull<IStream*> aStream);
+StripHandlerFromOBJREF(NotNull<IStream*> aStream,
+                       const uint64_t aStart,
+                       const uint64_t aEnd);
 
 } // namespace mscom
 } // namespace mozilla

ipc/mscom/oop/Handler.cpp

@@ -206,16 +206,16 @@ Handler::MarshalInterface(IStream* pStm, REFIID riid, void* pv,
   }
 
 #if defined(MOZ_MSCOM_REMARSHAL_NO_HANDLER)
-  // Now the OBJREF has been written, so seek back to its beginning (the
-  // position that we saved earlier).
-  seekTo.QuadPart = objrefPos.QuadPart;
-  hr = pStm->Seek(seekTo, STREAM_SEEK_SET, nullptr);
+  // Obtain the current stream position which is the end of the OBJREF
+  ULARGE_INTEGER endPos;
+  hr = pStm->Seek(seekTo, STREAM_SEEK_CUR, &endPos);
   if (FAILED(hr)) {
     return hr;
   }
 
   // Now strip out the handler.
-  if (!StripHandlerFromOBJREF(WrapNotNull(pStm))) {
+  if (!StripHandlerFromOBJREF(WrapNotNull(pStm), objrefPos.QuadPart,
+                              endPos.QuadPart)) {
     return E_FAIL;
   }