Changeset 83286 in vbox

Timestamp:

Mar 13, 2020 3:35:35 PM (5 years ago)

Author:

vboxsync

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

136442

Message:

VBoxService / Guest Control: Fixed guest process thread teardown / cleanup handling resulting in handle/thread leaks, separated functions more for cleaner structure. bugref:9135

Location:

trunk/src/VBox/Additions/common/VBoxService

Files:

• VBoxServiceControl.h (modified) (3 diffs)
• VBoxServiceControlProcess.cpp (modified) (13 diffs)
• VBoxServiceControlSession.cpp (modified) (12 diffs)

trunk/src/VBox/Additions/common/VBoxService/VBoxServiceControl.h

@@ -179 +179 @@
      *  (VBOXSERVICECTRLPROCESS). */
     RTLISTANCHOR                    lstProcesses;
+    /** Number of guest processes in the process list. */
+    uint32_t                        cProcesses;
     /** List of guest control files (VBOXSERVICECTRLFILE). */
     RTLISTANCHOR                    lstFiles;
+    /** Number of guest files in the file list. */
+    uint32_t                        cFiles;
     /** The session's critical section. */
     RTCRITSECT                      CritSect;
@@ -278 +282 @@
     RTPIPE                          hPipeStdOutR;
     /** StdOut pipe for addressing reads from
-     *  guest process' stdout.*/
+     *  guest process' stderr.*/
     RTPIPE                          hPipeStdErrR;
 
@@ -317 +321 @@
 extern int                      VGSvcGstCtrlSessionProcessAdd(PVBOXSERVICECTRLSESSION pSession, PVBOXSERVICECTRLPROCESS pProcess);
 extern int                      VGSvcGstCtrlSessionProcessRemove(PVBOXSERVICECTRLSESSION pSession, PVBOXSERVICECTRLPROCESS pProcess);
-extern int                      VGSvcGstCtrlSessionProcessStartAllowed(const PVBOXSERVICECTRLSESSION pSession, bool *pbAllowed);
+extern int                      VGSvcGstCtrlSessionProcessStartAllowed(const PVBOXSERVICECTRLSESSION pSession, bool *pfAllowed);
 extern int                      VGSvcGstCtrlSessionReapProcesses(PVBOXSERVICECTRLSESSION pSession);
 /** @} */

trunk/src/VBox/Additions/common/VBoxService/VBoxServiceControlProcess.cpp

@@ -136 +136 @@
  * @return  IPRT status code.
  * @param   pProcess                Guest process to free.
+ *                                  The pointer will not be valid anymore after return.
  */
 int VGSvcGstCtrlProcessFree(PVBOXSERVICECTRLPROCESS pProcess)
@@ -141 +142 @@
     AssertPtrReturn(pProcess, VERR_INVALID_POINTER);
 
-    VGSvcVerbose(3, "[PID %RU32]: Freeing (cRefs=%RU32)...\n", pProcess->uPID, pProcess->cRefs);
-    Assert(pProcess->cRefs == 0);
-
-    /*
-     * Destroy other thread data.
-     */
-    if (RTCritSectIsInitialized(&pProcess->CritSect))
+    int rc = RTCritSectEnter(&pProcess->CritSect);
+    if (RT_SUCCESS(rc))
+    {
+        VGSvcVerbose(3, "[PID %RU32]: Freeing (cRefs=%RU32)...\n", pProcess->uPID, pProcess->cRefs);
+
+        AssertReturn(pProcess->cRefs == 0, VERR_WRONG_ORDER);
+        AssertReturn(pProcess->fStopped, VERR_WRONG_ORDER);
+        AssertReturn(pProcess->fShutdown, VERR_WRONG_ORDER);
+
+        /*
+         * Destroy other thread data.
+         */
+        int rc = RTPollSetDestroy(pProcess->hPollSet);
+        AssertRC(rc);
+
+        rc = RTReqQueueDestroy(pProcess->hReqQueue);
+        AssertRC(rc);
+
+        rc = RTPipeClose(pProcess->hNotificationPipeR);
+        AssertRC(rc);
+        rc = RTPipeClose(pProcess->hNotificationPipeW);
+        AssertRC(rc);
+
+        rc = RTPipeClose(pProcess->hPipeStdInW);
+        AssertRC(rc);
+        rc = RTPipeClose(pProcess->hPipeStdErrR);
+        AssertRC(rc);
+        rc = RTPipeClose(pProcess->hPipeStdOutR);
+        AssertRC(rc);
+
+        rc = RTCritSectLeave(&pProcess->CritSect);
+        AssertRC(rc);
+
         RTCritSectDelete(&pProcess->CritSect);
 
-    int rc = RTReqQueueDestroy(pProcess->hReqQueue);
-    AssertRC(rc);
-
-    /*
-     * Remove from list.
-     */
-    AssertPtr(pProcess->pSession);
-    rc = VGSvcGstCtrlSessionProcessRemove(pProcess->pSession, pProcess);
-    AssertRC(rc);
-
-    /*
-     * Destroy thread structure as final step.
-     */
-    RTMemFree(pProcess);
-    pProcess = NULL;
-
-    return VINF_SUCCESS;
+        /*
+         * Destroy thread structure as final step.
+         */
+        RTMemFree(pProcess);
+        pProcess = NULL;
+    }
+
+    return rc;
 }
 
@@ -193 +211 @@
  * Releases a previously acquired guest process (decreases the refcount).
  *
- * @param   pProcess            Process to unlock.
+ * @param   pProcess            Process to release.
  */
 void VGSvcGstCtrlProcessRelease(PVBOXSERVICECTRLPROCESS pProcess)
@@ -199 +217 @@
     AssertPtrReturnVoid(pProcess);
 
-    bool fShutdown = false;
-
-    int rc = RTCritSectEnter(&pProcess->CritSect);
-    if (RT_SUCCESS(rc))
-    {
-        Assert(pProcess->cRefs);
+    int rc2 = RTCritSectEnter(&pProcess->CritSect);
+    if (RT_SUCCESS(rc2))
+    {
+        AssertReturnVoid(pProcess->cRefs);
         pProcess->cRefs--;
-        fShutdown = pProcess->fStopped; /* Has the process' thread been stopped? */
-
-        rc = RTCritSectLeave(&pProcess->CritSect);
-        AssertRC(rc);
-    }
-
-    if (fShutdown)
-        VGSvcGstCtrlProcessFree(pProcess);
+
+        VGSvcVerbose(3, "[PID %RU32]: cRefs=%RU32, fShutdown=%RTbool, fStopped=%RTbool\n",
+                     pProcess->uPID, pProcess->cRefs, pProcess->fShutdown, pProcess->fStopped);
+
+        rc2 = RTCritSectLeave(&pProcess->CritSect);
+        AssertRC(rc2);
+    }
 }
 
@@ -232 +247 @@
 
     int rc = vgsvcGstCtrlProcessLock(pProcess);
-    if (RT_SUCCESS(rc))
+    if (   RT_SUCCESS(rc)
+        && pProcess->Thread != NIL_RTTHREAD) /* Is there a thread we can wait for? */
     {
         VGSvcVerbose(2, "[PID %RU32]: Waiting for shutdown (%RU32ms) ...\n", pProcess->uPID, msTimeout);
@@ -240 +256 @@
                          pProcess, pProcess->uPID), VERR_INVALID_PARAMETER);
 
-        /* Guest process already has been stopped, no need to wait. */
-        if (!pProcess->fStopped)
-        {
-            /* Unlock process before waiting. */
-            rc = vgsvcGstCtrlProcessUnlock(pProcess);
-            AssertRC(rc);
-
-            /* Do the actual waiting. */
-            int rcThread;
-            Assert(pProcess->Thread != NIL_RTTHREAD);
-            rc = RTThreadWait(pProcess->Thread, msTimeout, &rcThread);
-            if (RT_SUCCESS(rc))
-            {
-                pProcess->Thread = NIL_RTTHREAD;
-                VGSvcVerbose(3, "[PID %RU32]: Thread shutdown complete, thread rc=%Rrc\n", pProcess->uPID, rcThread);
-                if (prc)
-                    *prc = rcThread;
-            }
-            else
-                VGSvcError("[PID %RU32]: Waiting for shutting down thread returned error rc=%Rrc\n", pProcess->uPID, rc);
+        /* Unlock process before waiting. */
+        rc = vgsvcGstCtrlProcessUnlock(pProcess);
+        AssertRC(rc);
+
+        /* Do the actual waiting. */
+        int rcThread;
+        Assert(pProcess->Thread != NIL_RTTHREAD);
+        rc = RTThreadWait(pProcess->Thread, msTimeout, &rcThread);
+
+        int rc2 = vgsvcGstCtrlProcessLock(pProcess);
+        AssertRC(rc2);
+
+        if (RT_SUCCESS(rc))
+        {
+            pProcess->Thread = NIL_RTTHREAD;
+            VGSvcVerbose(3, "[PID %RU32]: Thread shutdown complete, thread rc=%Rrc\n", pProcess->uPID, rcThread);
+            if (prc)
+                *prc = rcThread;
         }
         else
-        {
-            VGSvcVerbose(3, "[PID %RU32]: Thread already shut down, no waiting needed\n", pProcess->uPID);
-
-            int rc2 = vgsvcGstCtrlProcessUnlock(pProcess);
-            AssertRC(rc2);
-        }
+            VGSvcError("[PID %RU32]: Waiting for shutting down thread returned error rc=%Rrc\n", pProcess->uPID, rc);
+
+        rc2 = vgsvcGstCtrlProcessUnlock(pProcess);
+        AssertRC(rc2);
     }
 
@@ -1454 +1466 @@
     if (RT_FAILURE(rc))
     {
-        VGSvcError("Errorwhile adding guest process '%s' (%p) to session process list, rc=%Rrc\n",
+        VGSvcError("Error while adding guest process '%s' (%p) to session process list, rc=%Rrc\n",
                    pProcess->StartupInfo.szCmd, pProcess, rc);
         RTThreadUserSignal(RTThreadSelf());
@@ -1610 +1622 @@
                                          * So, NIL the handles to avoid closing them again.
                                          */
-                                        /** @todo r=bird: Can't see how hNotificationPipeR could be closed here!  Found (and fixed)
-                                         * confused comments documenting hNotificationPipeW, probably related. */
                                         if (RT_FAILURE(RTPollSetQueryHandle(pProcess->hPollSet,
                                                                             VBOXSERVICECTRLPIPEID_IPC_NOTIFY, NULL)))
-                                        {
-                                            pProcess->hNotificationPipeR = NIL_RTPIPE;
                                             pProcess->hNotificationPipeW = NIL_RTPIPE;
-                                        }
                                         if (RT_FAILURE(RTPollSetQueryHandle(pProcess->hPollSet,
                                                                             VBOXSERVICECTRLPIPEID_STDERR, NULL)))
@@ -1630 +1637 @@
                                 }
                                 RTPollSetDestroy(pProcess->hPollSet);
+                                pProcess->hPollSet = NIL_RTPOLLSET;
 
                                 RTPipeClose(pProcess->hNotificationPipeR);
@@ -1638 +1646 @@
                             RTPipeClose(pProcess->hPipeStdErrR);
                             pProcess->hPipeStdErrR = NIL_RTPIPE;
-                            RTHandleClose(phStdErr);
+                            RTHandleClose(&hStdErr);
                             if (phStdErr)
                                 RTHandleClose(phStdErr);
@@ -1650 +1658 @@
                     RTPipeClose(pProcess->hPipeStdInW);
                     pProcess->hPipeStdInW = NIL_RTPIPE;
-                    RTHandleClose(phStdIn);
+                    RTHandleClose(&hStdIn);
+                    if (phStdIn)
+                        RTHandleClose(phStdIn);
                 }
             }
@@ -1669 +1679 @@
     }
 
+    /* Update stopped status. */
+    ASMAtomicWriteBool(&pProcess->fStopped, true);
+
     if (cArgs)
         RTGetOptArgvFree(papszArgs);
@@ -1676 +1689 @@
      * forever on this thread.
      */
-    if (RT_FAILURE(rc) && !fSignalled)
+    if (   RT_FAILURE(rc)
+        && !fSignalled)
+    {
         RTThreadUserSignal(RTThreadSelf());
-
-    VGSvcVerbose(3, "[PID %RU32]: Thread of process '%s' ended with rc=%Rrc\n",
-                 pProcess->uPID, pProcess->StartupInfo.szCmd, rc);
-
-    /* Finally, update stopped status. */
-    ASMAtomicWriteBool(&pProcess->fStopped, true);
+    }
+
+    /* Set shut down flag in case we've forgotten it. */
     ASMAtomicWriteBool(&pProcess->fShutdown, true);
+
+    VGSvcVerbose(3, "[PID %RU32]: Thread of process '%s' ended with rc=%Rrc (fSignalled=%RTbool)\n",
+                 pProcess->uPID, pProcess->StartupInfo.szCmd, rc, fSignalled);
 
     return rc;

trunk/src/VBox/Additions/common/VBoxService/VBoxServiceControlSession.cpp

@@ -60 +60 @@
     VBOXSERVICESESSIONOPT_THREAD_ID
 };
+
+
+static int vgsvcGstCtrlSessionCleanupProcesses(const PVBOXSERVICECTRLSESSION pSession);
 
 
@@ -1054 +1057 @@
         if (RT_SUCCESS(rc))
         {
+            vgsvcGstCtrlSessionCleanupProcesses(pSession);
+
             if (fStartAllowed)
                 rc = VGSvcGstCtrlProcessStart(pSession, &startupInfo, pHostCtx->uContextID);
@@ -1226 +1231 @@
         {
             rc = VGSvcGstCtrlProcessHandleTerm(pProcess);
+            if (RT_FAILURE(rc))
+                VGSvcError("Error terminating PID=%RU32, rc=%Rrc\n", uPID, rc);
 
             VGSvcGstCtrlProcessRelease(pProcess);
@@ -1233 +1240 @@
             VGSvcError("Could not find PID %u for termination.\n", uPID);
             rc = VERR_PROCESS_NOT_FOUND;
-/** @todo r=bird:
- *
- *  No way to report status status code for output requests?
- *
- */
         }
     }
@@ -1838 +1840 @@
 
         /* Signal all guest processes in the active list that we want to shutdown. */
-        size_t cProcesses = 0;
         PVBOXSERVICECTRLPROCESS pProcess;
         RTListForEach(&pSession->lstProcesses, pProcess, VBOXSERVICECTRLPROCESS, Node)
-        {
             VGSvcGstCtrlProcessStop(pProcess);
-            cProcesses++;
-        }
-
-        VGSvcVerbose(1, "%zu guest processes were signalled to stop\n", cProcesses);
+
+        VGSvcVerbose(1, "%RU32 guest processes were signalled to stop\n", pSession->cProcesses);
 
         /* Wait for all active threads to shutdown and destroy the active thread list. */
@@ -1947 +1945 @@
     RTListInit(&pSession->lstFiles);
 
+    pSession->cProcesses = 0;
+    pSession->cFiles     = 0;
+
     pSession->fFlags = fFlags;
 
@@ -1972 +1973 @@
     if (RT_SUCCESS(rc))
     {
-        VGSvcVerbose( 3, "Adding process (PID %RU32) to session ID=%RU32\n", pProcess->uPID, pSession->StartupInfo.uSessionID);
+        VGSvcVerbose(3, "Adding process (PID %RU32) to session ID=%RU32\n", pProcess->uPID, pSession->StartupInfo.uSessionID);
 
         /* Add process to session list. */
         RTListAppend(&pSession->lstProcesses, &pProcess->Node);
+
+        pSession->cProcesses++;
+        VGSvcVerbose(3, "Now session ID=%RU32 has %RU32 processes total\n",
+                     pSession->StartupInfo.uSessionID, pSession->cProcesses);
 
         int rc2 = RTCritSectLeave(&pSession->CritSect);
@@ -2002 +2007 @@
     {
         VGSvcVerbose(3, "Removing process (PID %RU32) from session ID=%RU32\n", pProcess->uPID, pSession->StartupInfo.uSessionID);
-        Assert(pProcess->cRefs == 0);
+        AssertReturn(pProcess->cRefs == 0, VERR_WRONG_ORDER);
 
         RTListNodeRemove(&pProcess->Node);
+
+        AssertReturn(pSession->cProcesses, VERR_WRONG_ORDER);
+        pSession->cProcesses--;
+        VGSvcVerbose(3, "Now session ID=%RU32 has %RU32 processes total\n",
+                     pSession->StartupInfo.uSessionID, pSession->cProcesses);
 
         int rc2 = RTCritSectLeave(&pSession->CritSect);
@@ -2011 +2021 @@
     }
 
-    return VINF_SUCCESS;
+    return rc;
 }
 
@@ -2021 +2031 @@
  * @return  VBox status code.
  * @param   pSession            The guest session.
- * @param   pbAllowed           True if starting (another) guest process
- *                              is allowed, false if not.
+ * @param   pfAllowed           \c True if starting (another) guest process
+ *                              is allowed, \c false if not.
  */
-int VGSvcGstCtrlSessionProcessStartAllowed(const PVBOXSERVICECTRLSESSION pSession, bool *pbAllowed)
+int VGSvcGstCtrlSessionProcessStartAllowed(const PVBOXSERVICECTRLSESSION pSession, bool *pfAllowed)
 {
     AssertPtrReturn(pSession, VERR_INVALID_POINTER);
-    AssertPtrReturn(pbAllowed, VERR_INVALID_POINTER);
+    AssertPtrReturn(pfAllowed, VERR_INVALID_POINTER);
 
     int rc = RTCritSectEnter(&pSession->CritSect);
@@ -2039 +2049 @@
         if (pSession->uProcsMaxKept) /* If we allow unlimited processes (=0), take a shortcut. */
         {
-            uint32_t uProcsRunning = 0;
-            PVBOXSERVICECTRLPROCESS pProcess;
-            RTListForEach(&pSession->lstProcesses, pProcess, VBOXSERVICECTRLPROCESS, Node)
-                uProcsRunning++;
-
-            VGSvcVerbose(3, "Maximum served guest processes set to %u, running=%u\n", pSession->uProcsMaxKept, uProcsRunning);
-
-            int32_t iProcsLeft = (pSession->uProcsMaxKept - uProcsRunning - 1);
+            VGSvcVerbose(3, "Maximum kept guest processes set to %RU32, acurrent=%RU32\n",
+                         pSession->uProcsMaxKept, pSession->cProcesses);
+
+            int32_t iProcsLeft = (pSession->uProcsMaxKept - pSession->cProcesses - 1);
             if (iProcsLeft < 0)
             {
-                VGSvcVerbose(3, "Maximum running guest processes reached (%u)\n", pSession->uProcsMaxKept);
+                VGSvcVerbose(3, "Maximum running guest processes reached (%RU32)\n", pSession->uProcsMaxKept);
                 fLimitReached = true;
             }
         }
 
-        *pbAllowed = !fLimitReached;
+        *pfAllowed = !fLimitReached;
 
         int rc2 = RTCritSectLeave(&pSession->CritSect);
@@ -2060 +2066 @@
             rc = rc2;
     }
+
+    return rc;
+}
+
+
+/**
+ * Cleans up stopped and no longer used processes.
+ *
+ * This will free and remove processes from the session's process list.
+ *
+ * @returns VBox status code.
+ * @param   pSession            Session to clean up processes for.
+ */
+static int vgsvcGstCtrlSessionCleanupProcesses(const PVBOXSERVICECTRLSESSION pSession)
+{
+    AssertPtrReturn(pSession, VERR_INVALID_POINTER);
+
+    VGSvcVerbose(3, "Cleaning up stopped processes for session %RU32 ...\n", pSession->StartupInfo.uSessionID);
+
+    int rc2 = RTCritSectEnter(&pSession->CritSect);
+    AssertRC(rc2);
+
+    int rc = VINF_SUCCESS;
+
+    PVBOXSERVICECTRLPROCESS pCurProcess, pNextProcess;
+    RTListForEachSafe(&pSession->lstProcesses, pCurProcess, pNextProcess, VBOXSERVICECTRLPROCESS, Node)
+    {
+        if (ASMAtomicReadBool(&pCurProcess->fStopped))
+        {
+            rc2 = RTCritSectLeave(&pSession->CritSect);
+            AssertRC(rc2);
+
+            rc = VGSvcGstCtrlProcessWait(pCurProcess, 30 * 1000 /* Wait 30 seconds max. */, NULL /* rc */);
+            if (RT_SUCCESS(rc))
+            {
+                VGSvcGstCtrlSessionProcessRemove(pSession, pCurProcess);
+                VGSvcGstCtrlProcessFree(pCurProcess);
+            }
+
+            rc2 = RTCritSectEnter(&pSession->CritSect);
+            AssertRC(rc2);
+
+            /* If failed, try next time we're being called. */
+        }
+    }
+
+    rc2 = RTCritSectLeave(&pSession->CritSect);
+    AssertRC(rc2);
+
+    if (RT_FAILURE(rc))
+        VGSvcError("Cleaning up stopped processes for session %RU32 failed with %Rrc\n", pSession->StartupInfo.uSessionID, rc);
 
     return rc;