Changeset 70287 in vbox for trunk/src/VBox

Timestamp:

Dec 21, 2017 3:52:26 PM (7 years ago)

Author:

vboxsync

Message:

VBoxGuest-win.cpp: Snuffed out race between idle checking and session creation.

File:

• trunk/src/VBox/Additions/common/VBoxGuest/VBoxGuest-win.cpp (modified) (16 diffs)

trunk/src/VBox/Additions/common/VBoxGuest/VBoxGuest-win.cpp

@@ -38 +38 @@
 #include <iprt/asm.h>
 #include <iprt/asm-amd64-x86.h>
+#include <iprt/critsect.h>
 #include <iprt/dbg.h>
 #include <iprt/initterm.h>
@@ -135 +136 @@
      *  in a DPC callback and not the ISR. */
     KSPIN_LOCK              MouseEventAccessSpinLock;
+
+    /** Read/write critical section for handling race between checking for idle
+     * driver (in IRP_MN_QUERY_REMOVE_DEVICE & IRP_MN_QUERY_STOP_DEVICE) and
+     * creating new sessions.  The session creation code enteres the critical
+     * section in  read (shared) access mode, whereas the idle checking code
+     * enteres is in write (exclusive) access mode.  */
+    RTCRITSECTRW            SessionCreateCritSect;
 } VBOXGUESTDEVEXTWIN;
 typedef VBOXGUESTDEVEXTWIN *PVBOXGUESTDEVEXTWIN;
@@ -449 +457 @@
 
 
+/**
+ * Does the fundamental device extension initialization.
+ *
+ * @returns NT status.
+ * @param   pDevExt             The device extension.
+ * @param   pDevObj             The device object.
+ */
+static NTSTATUS vgdrvNtInitDevExtFundament(PVBOXGUESTDEVEXTWIN pDevExt, PDEVICE_OBJECT pDevObj)
+{
+    RT_ZERO(*pDevExt);
+
+    KeInitializeSpinLock(&pDevExt->MouseEventAccessSpinLock);
+    pDevExt->pDeviceObject   = pDevObj;
+    pDevExt->enmPrevDevState = VGDRVNTDEVSTATE_STOPPED;
+    pDevExt->enmDevState     = VGDRVNTDEVSTATE_STOPPED;
+
+    int rc = RTCritSectRwInit(&pDevExt->SessionCreateCritSect);
+    if (RT_SUCCESS(rc))
+    {
+        rc = VGDrvCommonInitDevExtFundament(&pDevExt->Core);
+        if (RT_SUCCESS(rc))
+        {
+            LogFlow(("vgdrvNtInitDevExtFundament: returning success\n"));
+            return STATUS_SUCCESS;
+        }
+
+        RTCritSectRwDelete(&pDevExt->SessionCreateCritSect);
+    }
+    Log(("vgdrvNtInitDevExtFundament: failed: rc=%Rrc\n", rc));
+    return STATUS_UNSUCCESSFUL;
+}
+
+
+/**
+ * Counter part to vgdrvNtInitDevExtFundament.
+ *
+ * @param   pDevExt             The device extension.
+ */
+static void vgdrvNtDeleteDevExtFundament(PVBOXGUESTDEVEXTWIN pDevExt)
+{
+    LogFlow(("vgdrvNtDeleteDevExtFundament:\n"));
+    VGDrvCommonDeleteDevExtFundament(&pDevExt->Core);
+    RTCritSectRwDelete(&pDevExt->SessionCreateCritSect);
+}
+
+
 #ifdef LOG_ENABLED
 /**
@@ -986 +1040 @@
             Log(("vgdrvNt4CreateDevice: Setting up device extension ...\n"));
             PVBOXGUESTDEVEXTWIN pDevExt = (PVBOXGUESTDEVEXTWIN)pDeviceObject->DeviceExtension;
-            RT_ZERO(*pDevExt);
-
-            /* Store a reference to ourself. */
-            pDevExt->pDeviceObject = pDeviceObject;
-
-            /* Store bus and slot number we've queried before. */
-            pDevExt->uBus  = uBus;
-            pDevExt->uSlot = uSlot.u.AsULONG;
-
-            /* Initialize common bits. */
-            int vrc = VGDrvCommonInitDevExtFundament(&pDevExt->Core);
+            int vrc = vgdrvNtInitDevExtFundament(pDevExt, pDeviceObject);
             if (RT_SUCCESS(vrc))
             {
+                /* Store bus and slot number we've queried before. */
+                pDevExt->uBus  = uBus;
+                pDevExt->uSlot = uSlot.u.AsULONG;
+
                 Log(("vgdrvNt4CreateDevice: Device extension created\n"));
 
@@ -1010 +1058 @@
 
                 /* bail out */
-                VGDrvCommonDeleteDevExtFundament(&pDevExt->Core);
+                vgdrvNtDeleteDevExtFundament(pDevExt);
             }
             IoDeleteSymbolicLink(&DosName);
@@ -1060 +1108 @@
              */
             PVBOXGUESTDEVEXTWIN pDevExt = (PVBOXGUESTDEVEXTWIN)pDeviceObject->DeviceExtension;
-            RT_ZERO(*pDevExt);
-
-            KeInitializeSpinLock(&pDevExt->MouseEventAccessSpinLock);
-            pDevExt->pDeviceObject   = pDeviceObject;
-            pDevExt->enmPrevDevState = VGDRVNTDEVSTATE_STOPPED;
-            pDevExt->enmDevState     = VGDRVNTDEVSTATE_STOPPED;
-
-            int vrc = VGDrvCommonInitDevExtFundament(&pDevExt->Core);
-            if (RT_SUCCESS(vrc))
+            rcNt = vgdrvNtInitDevExtFundament(pDevExt, pDeviceObject);
+            if (NT_SUCCESS(rcNt))
             {
                 pDevExt->pNextLowerDriver = IoAttachDeviceToDeviceStack(pDeviceObject, pDevObj);
@@ -1081 +1122 @@
                     return rcNt;
                 }
-
                 LogFunc(("IoAttachDeviceToDeviceStack did not give a nextLowerDriver!\n"));
                 rcNt = STATUS_DEVICE_NOT_CONNECTED;
-                VGDrvCommonDeleteDevExtFundament(&pDevExt->Core);
+                vgdrvNtDeleteDevExtFundament(pDevExt);
             }
-            else
-                rcNt = STATUS_UNSUCCESSFUL;
-
-            /* bail out */
+
             IoDeleteSymbolicLink(&DosName);
         }
@@ -1169 +1206 @@
         pDevExt->pInterruptObject = NULL;
     }
+    pDevExt->pPowerStateRequest = NULL; /* Will be deleted by the following call. */
     if (pDevExt->Core.uInitState == VBOXGUESTDEVEXT_INIT_STATE_RESOURCES)
         VGDrvCommonDeleteDevExtResources(&pDevExt->Core);
@@ -1189 +1227 @@
      * Delete the remainder of the device extension.
      */
-    pDevExt->pPowerStateRequest = NULL; /* Will be deleted by the following call. */
-    VGDrvCommonDeleteDevExtFundament(&pDevExt->Core);
+    vgdrvNtDeleteDevExtFundament(pDevExt);
 
     /*
@@ -1309 +1346 @@
             Log(("vgdrvNtNt5PlusPnP: QUERY_REMOVE_DEVICE\n"));
 
+            RTCritSectRwEnterExcl(&pDevExt->SessionCreateCritSect);
 #ifdef VBOX_REBOOT_ON_UNINSTALL
             Log(("vgdrvNtNt5PlusPnP: QUERY_REMOVE_DEVICE: Device cannot be removed without a reboot.\n"));
@@ -1318 +1356 @@
             {
                 pDevExt->enmDevState = VGDRVNTDEVSTATE_PENDINGREMOVE;
+                RTCritSectRwLeaveExcl(&pDevExt->SessionCreateCritSect);
 
                 /* This IRP passed down to lower driver. */
@@ -1332 +1371 @@
             else
             {
+                RTCritSectRwLeaveExcl(&pDevExt->SessionCreateCritSect);
                 pIrp->IoStatus.Status = rc;
                 IoCompleteRequest(pIrp, IO_NO_INCREMENT);
@@ -1429 +1469 @@
         {
             Log(("vgdrvNtNt5PlusPnP: QUERY_STOP_DEVICE\n"));
+            RTCritSectRwEnterExcl(&pDevExt->SessionCreateCritSect);
             rc = vgdrvNtCheckIdle(pDevExt, "QUERY_STOP_DEVICE");
             if (NT_SUCCESS(rc))
@@ -1434 +1475 @@
                 pDevExt->enmPrevDevState = pDevExt->enmDevState;
                 pDevExt->enmDevState = VGDRVNTDEVSTATE_PENDINGSTOP;
+                RTCritSectRwLeaveExcl(&pDevExt->SessionCreateCritSect);
 
                 /* This IRP passed down to lower driver. */
@@ -1449 +1491 @@
             else
             {
+                RTCritSectRwLeaveExcl(&pDevExt->SessionCreateCritSect);
                 pIrp->IoStatus.Status = rc;
                 IoCompleteRequest(pIrp, IO_NO_INCREMENT);
@@ -1836 +1879 @@
     /*
      * We are not remotely similar to a directory...
-     * (But this is possible.)
      */
-    if (pStack->Parameters.Create.Options & FILE_DIRECTORY_FILE)
+    NTSTATUS rcNt;
+    if (!(pStack->Parameters.Create.Options & FILE_DIRECTORY_FILE))
+    {
+        /*
+         * Check the device state.  We enter the critsect in shared mode to
+         * prevent race with PnP system requests checking whether we're idle.
+         */
+        RTCritSectRwEnterShared(&pDevExt->SessionCreateCritSect);
+        VGDRVNTDEVSTATE const enmDevState = pDevExt->enmDevState;
+        if (enmDevState == VGDRVNTDEVSTATE_OPERATIONAL)
+        {
+            /*
+             * Create a client session.
+             */
+            int                 rc;
+            PVBOXGUESTSESSION   pSession;
+            if (pIrp->RequestorMode == KernelMode)
+                rc = VGDrvCommonCreateKernelSession(&pDevExt->Core, &pSession);
+            else
+                rc = VGDrvCommonCreateUserSession(&pDevExt->Core, &pSession);
+            RTCritSectRwLeaveShared(&pDevExt->SessionCreateCritSect);
+            if (RT_SUCCESS(rc))
+            {
+                pFileObj->FsContext = pSession;
+                Log(("vgdrvNtCreate: Successfully created %s session %p\n",
+                     pIrp->RequestorMode == KernelMode ? "kernel" : "user", pSession));
+
+                return vgdrvNtCompleteRequestEx(STATUS_SUCCESS, FILE_OPENED, pIrp);
+            }
+
+            /* Note. the IoStatus is completely ignored on error. */
+            Log(("vgdrvNtCreate: Failed to create session: rc=%Rrc\n", rc));
+            if (rc == VERR_NO_MEMORY)
+                rcNt = STATUS_NO_MEMORY;
+            else
+                rcNt = STATUS_UNSUCCESSFUL;
+        }
+        else
+        {
+            RTCritSectRwLeaveShared(&pDevExt->SessionCreateCritSect);
+            LogFlow(("vgdrvNtCreate: Failed. Device is not in 'working' state: %d\n", enmDevState));
+            rcNt = STATUS_DEVICE_NOT_READY;
+        }
+    }
+    else
     {
         LogFlow(("vgdrvNtCreate: Failed. FILE_DIRECTORY_FILE set\n"));
-        return vgdrvNtCompleteRequest(STATUS_NOT_A_DIRECTORY, pIrp);
-    }
-
-    /*
-     * Check the device state.
-     */
-    if (pDevExt->enmDevState != VGDRVNTDEVSTATE_OPERATIONAL)
-    {
-        LogFlow(("vgdrvNtCreate: Failed. Device is not in 'working' state: %d\n", pDevExt->enmDevState));
-        return vgdrvNtCompleteRequest(STATUS_DEVICE_NOT_READY, pIrp);
-    }
-
-    /*
-     * Create a client session.
-     */
-    int                 rc;
-    PVBOXGUESTSESSION   pSession;
-    if (pIrp->RequestorMode == KernelMode)
-        rc = VGDrvCommonCreateKernelSession(&pDevExt->Core, &pSession);
-    else
-        rc = VGDrvCommonCreateUserSession(&pDevExt->Core, &pSession);
-    if (RT_SUCCESS(rc))
-    {
-        pFileObj->FsContext = pSession;
-        Log(("vgdrvNtCreate: Successfully created %s session %p\n",
-             pIrp->RequestorMode == KernelMode ? "kernel" : "user", pSession));
-        return vgdrvNtCompleteRequestEx(STATUS_SUCCESS, FILE_OPENED, pIrp);
-    }
-
-    Log(("vgdrvNtCreate: Failed to create session: rc=%Rrc\n", rc));
-    /* Note. the IoStatus is completely ignored on error. */
-    if (rc == VERR_NO_MEMORY)
-        return vgdrvNtCompleteRequest(STATUS_NO_MEMORY, pIrp);
-    return vgdrvNtCompleteRequest(STATUS_UNSUCCESSFUL, pIrp);
+        rcNt = STATUS_NOT_A_DIRECTORY;
+    }
+    return vgdrvNtCompleteRequest(rcNt, pIrp);
 }