Changeset 102533 in vbox for trunk/src

Timestamp:

Dec 8, 2023 10:15:02 AM (17 months ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

160677

Message:

Main/Guest Control: Fixed a shutdown race when uninitializing guest sessions via IGuestSession::Close(): There we need to check if its parent object (IGuest) still is around and in a working shape, and if not, do the cleanup stuff ourselves. Renamed Guest::i_sessionDestroy() -> Guest::i_sessionRemove() to make its function more clear. This race seemed to happen in combination with FE/Qt's guest file manager every now and then. bugref:9135

Location:

trunk/src/VBox/Main

Files:

• include/GuestImpl.h (modified) (1 diff)
• src-client/GuestCtrlImpl.cpp (modified) (2 diffs)
• src-client/GuestImpl.cpp (modified) (1 diff)
• src-client/GuestSessionImpl.cpp (modified) (5 diffs)

trunk/src/VBox/Main/include/GuestImpl.h ¶

@@ -131 +131 @@
     int         i_sessionCreate(const GuestSessionStartupInfo &ssInfo, const GuestCredentials &guestCreds,
                                 ComObjPtr<GuestSession> &pGuestSession);
-    int         i_sessionDestroy(uint32_t uSessionID);
+    int         i_sessionRemove(uint32_t uSessionID);
     inline bool i_sessionExists(uint32_t uSessionID);
     /** Returns the VBOX_GUESTCTRL_GF_0_XXX mask reported by the guest. */

trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp ¶

@@ -350 +350 @@
 
 /**
- * Destroys a given guest session and removes it from the internal list.
+ * Removes a given guest session and removes it from the internal list.
  *
  * @returns VBox status code.
  * @param   uSessionID          ID of the guest control session to destroy.
  *
- * @note    Takes the write lock.
+ * @note    Takes the read + write locks.
  */
-int Guest::i_sessionDestroy(uint32_t uSessionID)
+int Guest::i_sessionRemove(uint32_t uSessionID)
 {
     LogFlowThisFuncEnter();
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
     int vrc = VERR_NOT_FOUND;
 
-    LogFlowThisFunc(("Destroying session (ID=%RU32) ...\n", uSessionID));
+    LogFlowThisFunc(("Removing session (ID=%RU32) ...\n", uSessionID));
+
+    AutoReadLock arlock(this COMMA_LOCKVAL_SRC_POS);
 
     GuestSessions::iterator itSessions = mData.mGuestSessions.find(uSessionID);
@@ -379 +379 @@
 
     vrc = pSession->i_onRemove();
+
+    arlock.release();
+
+    AutoWriteLock awlock(this COMMA_LOCKVAL_SRC_POS);
     mData.mGuestSessions.erase(itSessions);
-
-    alock.release(); /* Release lock before firing off event. */
+    awlock.release(); /* Release write lock before firing off event. */
 
     ::FireGuestSessionRegisteredEvent(mEventSource, pSession, false /* Unregistered */);

trunk/src/VBox/Main/src-client/GuestImpl.cpp ¶

@@ -188 +188 @@
     {
 # ifdef DEBUG
-/** @todo r=bird: hit a use-after-free situation here while debugging the
- * 0xcccccccc status code issue in copyto.  My bet is that this happens
- * because of an uninit race, where GuestSession::close(), or someone, does
- * not ensure that the parent object (Guest) is okay to use (in the AutoCaller
- * sense), only their own object. */
         ULONG cRefs = itSessions->second->AddRef();
         LogFlowThisFunc(("sessionID=%RU32, cRefs=%RU32\n", itSessions->first, cRefs > 1 ? cRefs - 1 : 0));

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp ¶

@@ -320 +320 @@
 void GuestSession::uninit(void)
 {
+    LogFlowThisFuncEnter();
+
     /* Enclose the state transition Ready->InUninit->NotReady. */
     AutoUninitSpan autoUninitSpan(this);
     if (autoUninitSpan.uninitDone())
         return;
-
-    LogFlowThisFuncEnter();
 
     /* Call i_onRemove to take care of the object cleanups. */
@@ -3374 +3374 @@
 
 #ifndef VBOX_GUESTCTRL_TEST_CASE
+    AutoCaller autoCallerParent(mParent);
+    if (FAILED(autoCallerParent.hrc()))
+        return VERR_STATE_CHANGED;
+
     ComObjPtr<Console> pConsole = mParent->i_getConsole();
     Assert(!pConsole.isNull());
@@ -3379 +3383 @@
     /* Forward the information to the VMM device. */
     VMMDev *pVMMDev = pConsole->i_getVMMDev();
-    AssertPtrReturn(pVMMDev, VERR_STATE_CHANGED);
+    if (!pVMMDev)
+        return VERR_STATE_CHANGED;
 
     LogFlowThisFunc(("uMessage=%RU32 (%s), uParms=%RU32\n", uMessage, GstCtrlHostMsgtoStr((guestControl::eHostMsg)uMessage), uParms));
@@ -3851 +3856 @@
 HRESULT GuestSession::close()
 {
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.hrc())) return autoCaller.hrc();
+
     LogFlowThisFuncEnter();
 
@@ -3879 +3887 @@
      * work first and then return an error. */
 
-    /* Destroy session + remove ourselves from the session list. */
-    AssertPtr(mParent);
-    int vrc2 = mParent->i_sessionDestroy(mData.mSession.mID);
-    if (vrc2 == VERR_NOT_FOUND) /* Not finding the session anymore isn't critical. */
-        vrc2 = VINF_SUCCESS;
-
-    if (RT_SUCCESS(vrc))
-        vrc = vrc2;
+    /* We have to make sure that our parent (IGuest) still is alive and in a working shapee.
+     * If not, skip removing the session from it. */
+    AutoCaller autoCallerParent(mParent);
+    if (SUCCEEDED(autoCallerParent.hrc()))
+    {
+        LogFlowThisFunc(("Removing session '%s' from parent ...", mData.mSession.mName.c_str()));
+
+        /* Remove ourselves from the session list. */
+        AssertPtr(mParent);
+        int vrc2 = mParent->i_sessionRemove(mData.mSession.mID);
+        if (vrc2 == VERR_NOT_FOUND) /* Not finding the session anymore isn't critical. */
+            vrc2 = VINF_SUCCESS;
+
+        if (RT_SUCCESS(vrc))
+            vrc = vrc2;
+    }
+    else /* Do the session remove stuff ourselves. */
+        vrc = i_onRemove();
 
     LogFlowThisFunc(("Returning vrc=%Rrc, vrcGuest=%Rrc\n", vrc, vrcGuest));