Bug 1428684 - reduce the chance of UAF when changing states of MDSM. draft
authorJW Wang <jwwang@mozilla.com>
Mon, 08 Jan 2018 11:41:59 +0800
changeset 717059 5d9d0199107414a32c9ae950cbf3941436fd7530
parent 717058 98ba1a02b223821b39b9b0f331901cc3dce10c22
child 745137 028175642676c30de6500a01f65cd38078bb2a29
push id94547
push userjwwang@mozilla.com
push dateMon, 08 Jan 2018 06:05:39 +0000
bugs1428684
milestone59.0a1
Bug 1428684 - reduce the chance of UAF when changing states of MDSM. SetState() will delete the current state object and UAF will happen if members are accessed after this call. However, sometimes it is not obvious if SetState() is called indirectly as we do in MaybeFinishSeek(). To make it less error-prone, we will keep the old state object alive for a bit longer and set mMaster to null to catch potential UAF. MozReview-Commit-ID: Aqxj92ETjti
dom/media/MediaDecoderStateMachine.cpp
--- a/dom/media/MediaDecoderStateMachine.cpp
+++ b/dom/media/MediaDecoderStateMachine.cpp
@@ -285,29 +285,36 @@ protected:
     // |aArgs| must be passed by reference to avoid passing MOZ_NON_PARAM class
     // SeekJob by value.  See bug 1287006 and bug 1338374.  But we still *must*
     // copy the parameters, because |Exit()| can modify them.  See bug 1312321.
     // So we 1) pass the parameters by reference, but then 2) immediately copy
     // them into a Tuple to be safe against modification, and finally 3) move
     // the elements of the Tuple into the final function call.
     auto copiedArgs = MakeTuple(Forward<Ts>(aArgs)...);
 
-    // keep mMaster in a local object because mMaster will become invalid after
-    // the current state object is deleted.
+    // Copy mMaster which will reset to null.
     auto master = mMaster;
 
     auto* s = new S(master);
 
     MOZ_ASSERT(GetState() != s->GetState() ||
                GetState() == DECODER_STATE_SEEKING);
 
     SLOG("change state to: %s", ToStateStr(s->GetState()));
 
     Exit();
 
+    // Delete the old state asynchronously to avoid UAF if the caller tries to
+    // access its members after SetState() returns.
+    master->OwnerThread()->DispatchDirectTask(
+      NS_NewRunnableFunction("MDSM::StateObject::DeleteOldState",
+                             [toDelete = Move(master->mStateObj)](){}));
+    // Also reset mMaster to catch potentail UAF.
+    mMaster = nullptr;
+
     master->mStateObj.reset(s);
     return CallEnterMemberFunction(s, copiedArgs,
                                    typename IndexSequenceFor<Ts...>::Type());
   }
 
   RefPtr<MediaDecoder::SeekPromise>
   SetSeekingState(SeekJob&& aSeekJob, EventVisibility aVisibility);