Changeset 90228 in vbox

Timestamp:

Jul 16, 2021 11:30:51 AM (3 years ago)

Author:

vboxsync

Message:

HGCM: Applied fix for bugref:10038.

File:

• trunk/src/VBox/Main/src-client/HGCM.cpp (modified) (10 diffs)

trunk/src/VBox/Main/src-client/HGCM.cpp

@@ -72 +72 @@
     /* The service name follows. */
 };
+
+class HGCMClient;
 
 /** Internal helper service object. HGCM code would use it to
@@ -170 +172 @@
 
         int CreateAndConnectClient(uint32_t *pu32ClientIdOut, uint32_t u32ClientIdIn, uint32_t fRequestor, bool fRestoring);
-        int DisconnectClient(uint32_t u32ClientId, bool fFromService);
+        int DisconnectClient(uint32_t u32ClientId, bool fFromService, HGCMClient *pClient);
 
         int HostCall(uint32_t u32Function, uint32_t cParms, VBOXHGCMSVCPARM *paParms);
@@ -432 +434 @@
 {
     public:
-        /* client identifier */
+        /** client identifier */
         uint32_t u32ClientId;
+        /** The client instance. */
+        HGCMClient *pClient;
 };
 
@@ -632 +636 @@
                 HGCMMsgSvcDisconnect *pMsg = (HGCMMsgSvcDisconnect *)pMsgCore;
 
-                LogFlowFunc(("SVC_MSG_DISCONNECT u32ClientId = %d\n", pMsg->u32ClientId));
-
-                HGCMClient *pClient = (HGCMClient *)hgcmObjReference(pMsg->u32ClientId, HGCMOBJ_CLIENT);
-
-                if (pClient)
+                LogFlowFunc(("SVC_MSG_DISCONNECT u32ClientId = %d, pClient = %p\n", pMsg->u32ClientId, pMsg->pClient));
+
+                if (pMsg->pClient)
                 {
                     rc = pSvc->m_fntable.pfnDisconnect(pSvc->m_fntable.pvService, pMsg->u32ClientId,
-                                                       HGCM_CLIENT_DATA(pSvc, pClient));
-
-                    hgcmObjDereference(pClient);
+                                                       HGCM_CLIENT_DATA(pSvc, pMsg->pClient));
                 }
                 else
@@ -868 +868 @@
      HGCMService *pService = static_cast <HGCMService *> (pvInstance);
 
+     AssertMsgFailed(("This is unused code with a serialization issue.\n"
+                      "It touches data which is normally serialized by only running on the HGCM thread!\n"));
+
      if (pService)
      {
-         pService->DisconnectClient(u32ClientId, true);
+         pService->DisconnectClient(u32ClientId, true, NULL);
      }
 }
@@ -1380 +1383 @@
         while (pSvc->m_cClients && pSvc->m_paClientIds)
         {
-            LogFlowFunc(("handle %d\n", pSvc->m_paClientIds[0]));
-            pSvc->DisconnectClient(pSvc->m_paClientIds[0], false);
+            uint32_t const     idClient = pSvc->m_paClientIds[0];
+            HGCMClient * const pClient  = (HGCMClient *)hgcmObjReference(idClient, HGCMOBJ_CLIENT);
+            Assert(pClient);
+            LogFlowFunc(("handle %d/%p\n", pSvc->m_paClientIds[0], pClient));
+
+            pSvc->DisconnectClient(pSvc->m_paClientIds[0], false, pClient);
+
+            hgcmObjDereference(pClient);
         }
 
@@ -1648 +1657 @@
 }
 
-/* Disconnect the client from the service and delete the client handle.
- *
- * @param u32ClientId  The handle of the client.
- * @return VBox rc.
- */
-int HGCMService::DisconnectClient(uint32_t u32ClientId, bool fFromService)
-{
-    int rc = VINF_SUCCESS;
-
-    LogFlowFunc(("client id = %d, fFromService = %d\n", u32ClientId, fFromService));
-
+/**
+ * Disconnect the client from the service and delete the client handle.
+ *
+ * @param   u32ClientId     The handle of the client.
+ * @param   fFromService    Set if called by the service via
+ *                          svcHlpDisconnectClient().  pClient can be NULL when
+ *                          this is @c true.
+ * @param   pClient         The client disconnecting.  NULL if from service.
+ * @return  VBox status code.
+ */
+int HGCMService::DisconnectClient(uint32_t u32ClientId, bool fFromService, HGCMClient *pClient)
+{
+    Assert(pClient || !fFromService);
+
+    LogFlowFunc(("client id = %d, fFromService = %d, pClient = %p\n", u32ClientId, fFromService, pClient));
+
+    /*
+     * Destroy the client handle prior to the disconnecting to avoid creating
+     * a race with other messages from the same client.  See @bugref{10038}
+     * for further details. 
+     */
+    bool fReleaseService = false;
+    int  rc              = VERR_NOT_FOUND;
+    for (uint32_t i = 0; i < m_cClients; i++)
+    {
+        if (m_paClientIds[i] == u32ClientId)
+        {
+            m_cClients--;
+
+            if (m_cClients > i)
+                memmove(&m_paClientIds[i], &m_paClientIds[i + 1], sizeof(m_paClientIds[0]) * (m_cClients - i));
+
+            /* Delete the client handle. */
+            hgcmObjDeleteHandle(u32ClientId);
+            fReleaseService = true;
+
+            rc = VINF_SUCCESS;
+            break;
+        }
+    }
+
+    /* Some paranoia wrt to not trusting the client ID array. */
+    Assert(rc == VINF_SUCCESS || fFromService);
+    if (rc == VERR_NOT_FOUND && !fFromService)
+    {
+        hgcmObjDeleteHandle(u32ClientId);
+        fReleaseService = true;
+    }
+
+    /*
+     * Call the service.
+     */
     if (!fFromService)
     {
@@ -1671 +1721 @@
 
             pMsg->u32ClientId = u32ClientId;
+            pMsg->pClient = pClient;
 
             rc = hgcmMsgSend(pMsg);
@@ -1681 +1732 @@
     }
 
-    /* Remove the client id from the array in any case, rc does not matter. */
-    uint32_t i;
-
-    for (i = 0; i < m_cClients; i++)
-    {
-        if (m_paClientIds[i] == u32ClientId)
-        {
-            m_cClients--;
-
-            if (m_cClients > i)
-                memmove(&m_paClientIds[i], &m_paClientIds[i + 1], sizeof(m_paClientIds[0]) * (m_cClients - i));
-
-            /* Delete the client handle. */
-            hgcmObjDeleteHandle(u32ClientId);
-
-            /* The service must be released. */
-            ReleaseService();
-
-            break;
-        }
-    }
+
+    /*
+     * Release the pClient->pService reference.
+     */
+    if (fReleaseService)
+        ReleaseService();
 
     LogFlowFunc(("rc = %Rrc\n", rc));
@@ -2089 +2125 @@
 
                 /* Call the service instance to disconnect the client. */
-                rc = pService->DisconnectClient(pMsg->u32ClientId, false);
+                rc = pService->DisconnectClient(pMsg->u32ClientId, false, pClient);
 
                 hgcmObjDereference(pClient);