[vbox-dev] vgdrvIoCtl_CancelAllWaitEvents inconsistent behavior ?

[Michael Thayer]

Hello Hans,

03.07.2017 20:49, Michael Thayer wrote:
[Discussion of incorrect looking wait code in VBoxGuest.cpp.]>
> I did not get any response when I asked around about this.  Reading the
> code (quite a bit of work, as you presumably discovered too; fortunately
> the user-space part is a bit easier) suggests that this is not intended,
> as I could not find a caller in trunk or versions 5.1, 5.0 or 4.3 which
> depended on this behaviour, and one of the few callers that I did find
> (src/VBox/Additions/common/VBoxService/VBoxServiceCpuHotPlug.cpp) looks
> like it would handle it incorrectly.  I will correct this in our code
> too unless someone here speaks up.

I am testing out the patch below.  I can only see one caller site where
this path might trigger in a very unlikely race.  I will put a sleep and
a LogRel in there to compare the results with and without the change.

Regards
Michael

Index: src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp
===================================================================
--- src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp	(revision 116649)
+++ src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp	(working copy)
@@ -1584,7 +1584,7 @@
     uint32_t fMatches = pDevExt->f32PendingEvents & fReqEvents;
     if (fMatches & VBOXGUEST_ACQUIRE_STYLE_EVENTS)
         fMatches &= vgdrvGetAllowedEventMaskForSession(pDevExt, pSession);
-    if (fMatches || pSession->fPendingCancelWaitEvents)
+    if (fMatches)
     {
         ASMAtomicAndU32(&pDevExt->f32PendingEvents, ~fMatches);
         RTSpinlockRelease(pDevExt->EventSpinlock);
@@ -1598,6 +1598,13 @@
         pSession->fPendingCancelWaitEvents = false;
         return VINF_SUCCESS;
     }
+    else if (pSession->fPendingCancelWaitEvents)
+    {
+        RTSpinlockRelease(pDevExt->EventSpinlock);
+        pInfo->u32Result = VBOXGUEST_WAITEVENT_INTERRUPTED;
+        pSession->fPendingCancelWaitEvents = false;
+        return VINF_INTERRUPTED;
+    }

     RTSpinlockRelease(pDevExt->EventSpinlock);
     return VERR_TIMEOUT;
@@ -1633,7 +1640,7 @@
      */
     RTSpinlockAcquire(pDevExt->EventSpinlock);
     rc = vbdgCheckWaitEventCondition(pDevExt, pSession, pInfo, iEvent,
fReqEvents);
-    if (rc == VINF_SUCCESS)
+    if (rc == VINF_SUCCESS || rc == VERR_INTERRUPTED)
         return rc;

     if (!pInfo->u32TimeoutIn)
@@ -1656,7 +1663,7 @@
     RTSpinlockAcquire(pDevExt->EventSpinlock);
     RTListAppend(&pDevExt->WaitList, &pWait->ListNode);
     rc = vbdgCheckWaitEventCondition(pDevExt, pSession, pInfo, iEvent,
fReqEvents);
-    if (rc == VINF_SUCCESS)
+    if (rc == VINF_SUCCESS || rc == VERR_INTERRUPTED)
     {
         vgdrvWaitFreeUnlocked(pDevExt, pWait);
         return rc;
-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher