Bug 1459085 - Prevent mutex reentry in mscom::Interceptor::GetInterceptorForIID. r=Jamie, a=jcristau
authorAaron Klotz <aklotz@mozilla.com>
Thu, 03 May 2018 22:57:11 -0600
changeset 802194 f10bc261be65712b1d508bf7b7b0750f2c66d161
parent 802193 5dc9b3739510ed74b72c91c311f5647c41572fe1
child 802195 f7ba2965406d08645df693bfe3ce8b798a512915
push id111850
push userbmo:tom@mozilla.com
push dateThu, 31 May 2018 16:41:37 +0000
reviewersJamie, jcristau
bugs1459085
milestone60.0.2
Bug 1459085 - Prevent mutex reentry in mscom::Interceptor::GetInterceptorForIID. r=Jamie, a=jcristau
ipc/mscom/Interceptor.cpp
ipc/mscom/Interceptor.h
--- a/ipc/mscom/Interceptor.cpp
+++ b/ipc/mscom/Interceptor.cpp
@@ -580,31 +580,40 @@ Interceptor::GetInitialInterceptorForIID
   ENSURE_HR_SUCCEEDED(hr);
 
   if (MarshalAs(aTargetIid) == aTargetIid) {
     hr = unkInterceptor->QueryInterface(aTargetIid, aOutInterceptor);
     ENSURE_HR_SUCCEEDED(hr);
     return hr;
   }
 
-  hr = GetInterceptorForIID(aTargetIid, aOutInterceptor);
+  hr = GetInterceptorForIID(aTargetIid, aOutInterceptor, &lock);
   ENSURE_HR_SUCCEEDED(hr);
   return hr;
 }
 
+HRESULT
+Interceptor::GetInterceptorForIID(REFIID aIid, void** aOutInterceptor)
+{
+  return GetInterceptorForIID(aIid, aOutInterceptor, nullptr);
+}
+
 /**
  * This method contains the core guts of the handling of QueryInterface calls
  * that are delegated to us from the ICallInterceptor.
  *
  * @param aIid ID of the desired interface
  * @param aOutInterceptor The resulting emulated vtable that corresponds to
  * the interface specified by aIid.
+ * @param aAlreadyLocked Proof of an existing lock on |mInterceptorMapMutex|,
+ *                       if present.
  */
 HRESULT
-Interceptor::GetInterceptorForIID(REFIID aIid, void** aOutInterceptor)
+Interceptor::GetInterceptorForIID(REFIID aIid, void** aOutInterceptor,
+                                  MutexAutoLock* aAlreadyLocked)
 {
   detail::LoggedQIResult result(aIid);
 
   if (!aOutInterceptor) {
     return E_INVALIDARG;
   }
 
   if (aIid == IID_IUnknown) {
@@ -616,24 +625,29 @@ Interceptor::GetInterceptorForIID(REFIID
 
   REFIID interceptorIid = MarshalAs(aIid);
 
   RefPtr<IUnknown> unkInterceptor;
   IUnknown* interfaceForQILog = nullptr;
 
   // (1) Check to see if we already have an existing interceptor for
   // interceptorIid.
-
-  { // Scope for lock
-    MutexAutoLock lock(mInterceptorMapMutex);
+  auto doLookup = [&]() -> void {
     MapEntry* entry = Lookup(interceptorIid);
     if (entry) {
       unkInterceptor = entry->mInterceptor;
       interfaceForQILog = entry->mTargetInterface;
     }
+  };
+
+  if (aAlreadyLocked) {
+    doLookup();
+  } else {
+    MutexAutoLock lock(mInterceptorMapMutex);
+    doLookup();
   }
 
   // (1a) A COM interceptor already exists for this interface, so all we need
   // to do is run a QI on it.
   if (unkInterceptor) {
     // Technically we didn't actually execute a QI on the target interface, but
     // for logging purposes we would like to record the fact that this interface
     // was requested.
@@ -683,19 +697,17 @@ Interceptor::GetInterceptorForIID(REFIID
   hr = unkInterceptor->QueryInterface(IID_ICallInterceptor,
                                       (void**)getter_AddRefs(interceptor));
   ENSURE_HR_SUCCEEDED(hr);
 
   hr = interceptor->RegisterSink(mEventSink);
   ENSURE_HR_SUCCEEDED(hr);
 
   // (5) Now that we have this new COM interceptor, insert it into the map.
-
-  { // Scope for lock
-    MutexAutoLock lock(mInterceptorMapMutex);
+  auto doInsertion = [&]() -> void {
     // We might have raced with another thread, so first check that we don't
     // already have an entry for this
     MapEntry* entry = Lookup(interceptorIid);
     if (entry && entry->mInterceptor) {
       // Bug 1433046: Because of aggregation, the QI for |interceptor|
       // AddRefed |this|, not |unkInterceptor|. Thus, releasing |unkInterceptor|
       // will destroy the object. Before we do that, we must first release
       // |interceptor|. Otherwise, |interceptor| would be invalidated when
@@ -706,16 +718,23 @@ Interceptor::GetInterceptorForIID(REFIID
       // MapEntry has a RefPtr to unkInterceptor, OTOH we must not touch the
       // refcount for the target interface because we are just moving it into
       // the map and its refcounting might not be thread-safe.
       IUnknown* rawTargetInterface = targetInterface.release();
       mInterceptorMap.AppendElement(MapEntry(interceptorIid,
                                              unkInterceptor,
                                              rawTargetInterface));
     }
+  };
+
+  if (aAlreadyLocked) {
+    doInsertion();
+  } else {
+    MutexAutoLock lock(mInterceptorMapMutex);
+    doInsertion();
   }
 
   hr = unkInterceptor->QueryInterface(interceptorIid, aOutInterceptor);
   ENSURE_HR_SUCCEEDED(hr);
   return hr;
 }
 
 HRESULT
@@ -830,17 +849,17 @@ Interceptor::WeakRefQueryInterface(REFII
     IDispatch* rawDisp = nullptr;
     HRESULT hr = QueryInterfaceTarget(aIid, (void**)&rawDisp);
     ENSURE_HR_SUCCEEDED(hr);
 
     disp.reset(rawDisp);
     return DispatchForwarder::Create(this, disp, aOutInterface);
   }
 
-  return GetInterceptorForIID(aIid, (void**)aOutInterface);
+  return GetInterceptorForIID(aIid, (void**)aOutInterface, nullptr);
 }
 
 ULONG
 Interceptor::AddRef()
 {
   return WeakReferenceSupport::AddRef();
 }
 
--- a/ipc/mscom/Interceptor.h
+++ b/ipc/mscom/Interceptor.h
@@ -139,16 +139,18 @@ private:
 
 private:
   explicit Interceptor(IInterceptorSink* aSink);
   ~Interceptor();
   HRESULT GetInitialInterceptorForIID(detail::LiveSetAutoLock& aLiveSetLock,
                                       REFIID aTargetIid,
                                       STAUniquePtr<IUnknown> aTarget,
                                       void** aOutInterface);
+  HRESULT GetInterceptorForIID(REFIID aIid, void** aOutInterceptor,
+                               MutexAutoLock* aAlreadyLocked);
   MapEntry* Lookup(REFIID aIid);
   HRESULT QueryInterfaceTarget(REFIID aIid, void** aOutput,
                                TimeDuration* aOutDuration = nullptr);
   HRESULT WeakRefQueryInterface(REFIID aIid,
                                 IUnknown** aOutInterface) override;
   HRESULT CreateInterceptor(REFIID aIid, IUnknown* aOuter, IUnknown** aOutput);
   REFIID MarshalAs(REFIID aIid) const;
   HRESULT PublishTarget(detail::LiveSetAutoLock& aLiveSetLock,