Changeset 43478 in vbox for trunk/src/VBox/Main

Timestamp:

Oct 1, 2012 9:05:19 AM (12 years ago)

Author:

vboxsync

Message:

Main/Medium: fix race (involving several locks and the object state/caller) between VirtualBox::OpenMedium and Medium::saveSettings. Very tiny window, can only happen after queryInfo has sorted it correctly into the media tree and before the medium was marked as initialized. Also eliminate a race between placing the image in the media tree and updating its UUID to the correct value.

File:

• trunk/src/VBox/Main/src-server/MediumImpl.cpp (modified) (4 diffs)

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

@@ -1019 +1019 @@
     AssertReturn(!aLocation.isEmpty(), E_INVALIDARG);
 
-    /* Enclose the state transition NotReady->InInit->Ready */
-    AutoInitSpan autoInitSpan(this);
-    AssertReturn(autoInitSpan.isOk(), E_FAIL);
-
     HRESULT rc = S_OK;
 
-    unconst(m->pVirtualBox) = aVirtualBox;
-
-    /* there must be a storage unit */
-    m->state = MediumState_Created;
-
-    /* remember device type for correct unregistering later */
-    m->devType = aDeviceType;
-
-    /* cannot be a host drive */
-    m->hostDrive = false;
-
-    /* remember the open mode (defaults to ReadWrite) */
-    m->hddOpenMode = enOpenMode;
-
-    if (aDeviceType == DeviceType_DVD)
-        m->type = MediumType_Readonly;
-    else if (aDeviceType == DeviceType_Floppy)
-        m->type = MediumType_Writethrough;
-
-    rc = setLocation(aLocation);
-    if (FAILED(rc)) return rc;
-
-    /* get all the information about the medium from the storage unit */
-    if (fForceNewUuid)
-        unconst(m->uuidImage).create();
-
+    {
+        /* Enclose the state transition NotReady->InInit->Ready */
+        AutoInitSpan autoInitSpan(this);
+        AssertReturn(autoInitSpan.isOk(), E_FAIL);
+
+        unconst(m->pVirtualBox) = aVirtualBox;
+
+        /* there must be a storage unit */
+        m->state = MediumState_Created;
+
+        /* remember device type for correct unregistering later */
+        m->devType = aDeviceType;
+
+        /* cannot be a host drive */
+        m->hostDrive = false;
+
+        /* remember the open mode (defaults to ReadWrite) */
+        m->hddOpenMode = enOpenMode;
+
+        if (aDeviceType == DeviceType_DVD)
+            m->type = MediumType_Readonly;
+        else if (aDeviceType == DeviceType_Floppy)
+            m->type = MediumType_Writethrough;
+
+        rc = setLocation(aLocation);
+        if (FAILED(rc)) return rc;
+
+        /* get all the information about the medium from the storage unit */
+        if (fForceNewUuid)
+            unconst(m->uuidImage).create();
+
+        m->state = MediumState_Inaccessible;
+        m->strLastAccessError = tr("Accessibility check was not yet performed");
+
+        /* Confirm a successful initialization before the call to queryInfo.
+         * Otherwise we can end up with a AutoCaller deadlock because the
+         * medium becomes visible but is not marked as initialized. Causes
+         * locking trouble (e.g. trying to save media registries) which is
+         * hard to solve. */
+        autoInitSpan.setSucceeded();
+    }
+
+    /* we're normal code from now on, no longer init */
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc()))
+        return autoCaller.rc();
+
+    /* need to call queryInfo immediately to correctly place the medium in
+     * the respective media tree and update other information such as uuid */
     rc = queryInfo(fForceNewUuid /* fSetImageId */, false /* fSetParentId */);
-
     if (SUCCEEDED(rc))
     {
+        AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
         /* if the storage unit is not accessible, it's not acceptable for the
          * newly opened media so convert this into an error */
@@ -1061 +1081 @@
             Assert(!m->strLastAccessError.isEmpty());
             rc = setError(E_FAIL, "%s", m->strLastAccessError.c_str());
+            alock.release();
+            autoCaller.release();
+            uninit();
         }
         else
         {
-            AssertReturn(!m->id.isEmpty(), E_FAIL);
+            AssertStmt(!m->id.isEmpty(),
+                       alock.release(); autoCaller.release(); uninit(); return E_FAIL);
 
             /* storage format must be detected by Medium::queryInfo if the
              * medium is accessible */
-            AssertReturn(!m->strFormat.isEmpty(), E_FAIL);
-        }
-    }
-
-    /* Confirm a successful initialization when it's the case */
-    if (SUCCEEDED(rc))
-        autoInitSpan.setSucceeded();
+            AssertStmt(!m->strFormat.isEmpty(),
+                       alock.release(); autoCaller.release(); uninit(); return E_FAIL);
+        }
+    }
+    else
+    {
+        /* opening this image failed, mark the object as dead */
+        autoCaller.release();
+        uninit();
+    }
 
     return rc;
@@ -5563 +5590 @@
             }
 
+            /* set the image uuid before the below parent uuid handling code
+             * might place it somewhere in the media tree, so that the medium
+             * UUID is valid at this point */
+            alock.acquire();
+            if (isImport || fSetImageId)
+                unconst(m->id) = mediumId;
+            alock.release();
+
             /* get the medium variant */
             unsigned uImageFlags;
@@ -5697 +5732 @@
     treeLock.acquire();
     alock.acquire();
-
-    if (isImport || fSetImageId)
-        unconst(m->id) = mediumId;
 
     if (success)