Changeset 98836 in vbox

Timestamp:

Mar 3, 2023 9:36:21 PM (21 months ago)

Author:

vboxsync

Message:

Main/VirtualBox: Machine::unregister() and VirtualBox::openMachine() can
deadlock while processing a VM's media registry when run concurrently
for the same VM. If openMachine() is registering a differencing hard
disk while unregister() is unregistering the same disk then the two
threads can deadlock. The openMachine() thread can block in
VirtualBox::i_registerMedium() waiting for the media tree lock which the
unregister() thread holds but the unregister() thread can then block in
Medium::i_removeRegistryAll() waiting for the medium object to move out
of the 'InInit' state which the openMachine() thread will do once it
acquires the media tree lock so they are deadlocked. The fix is to
close the window of opportunity in VirtualBox::i_registerMedium() by not
dropping the media tree lock when called by Medium::initFromSettings().
bugref:6447

File:

• trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp (modified) (3 diffs)

trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp

@@ -5137 +5137 @@
         if (devType == DeviceType_HardDisk)
             m->mapHardDisks[id] = pMedium;
-
-        mediumCaller.release();
-        mediaTreeLock.release();
-        *ppMedium = pMedium;
-    }
-    else
+    }
+
+    /*
+     * If we have been called from Medium::initFromSettings() then the Medium object's
+     * AutoCaller status will be 'InInit' which means that when making the assigment to
+     * ppMedium below the Medium object will not call Medium::uninit().  By excluding
+     * this code path from releasing and reacquiring the media tree lock we avoid a
+     * potential deadlock with other threads which may be operating on the
+     * disks/DVDs/floppies in the VM's media registry at the same time such as
+     * Machine::unregister().
+     */
+    if (!fCalledFromMediumInit)
     {
         // pMedium may be the last reference to the Medium object, and the
@@ -5153 +5159 @@
         // must not hold the media tree write lock any more
         Assert(!i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-        *ppMedium = pDupMedium;
-    }
+    }
+
+    *ppMedium = pDupMedium.isNull() ? pMedium : pDupMedium;
 
     if (fAddToGlobalRegistry)
@@ -5167 +5174 @@
     // Restore the initial lock state, so that no unexpected lock changes are
     // done by this method, which would need adjustments everywhere.
-    mediaTreeLock.acquire();
+    if (!fCalledFromMediumInit)
+        mediaTreeLock.acquire();
 
     return hrc;