Changeset 98846 in vbox for trunk

Timestamp:

Mar 6, 2023 6:56:23 PM (2 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

156169

Message:

Main/Medium+VirtualBox: If Medium::close() and VirtualBox::i_registerMedium()
are running concurrently they can wrestle over the media tree lock and
end up registering a medium which is in the process of being closed. If
both of the respective threads are operating on the same medium then there
is a window of opportunity when Medium::i_close() drops the media tree
lock before calling Medium::uninit() that VirtualBox::i_registerMedium()
can grab the lock and register the medium. The fix is to check whether
the medium is in the process of being closed inside i_registerMedium()
and bail out if so to maintain media registry consistency. bugref:6447

Location:

trunk/src/VBox/Main

Files:

• include/MediumImpl.h (modified) (1 diff)
• src-server/MediumImpl.cpp (modified) (4 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (1 diff)

trunk/src/VBox/Main/include/MediumImpl.h

@@ -117 +117 @@
     MediumVariant_T i_getVariant() const;
     bool i_isHostDrive() const;
+    bool i_isClosing() const;
     const Utf8Str& i_getLocationFull() const;
     const Utf8Str& i_getFormat() const;

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

@@ -1448 +1448 @@
                 break;
             hrc = aVirtualBox->i_registerMedium(pActualMedium, &pActualMedium, mediaTreeLock, true /*fCalledFromMediumInit*/);
-            if (FAILED(hrc))
-                break;
-
-            if (pActualMedium == pMedium)
+            if (SUCCEEDED(hrc) && pActualMedium == pMedium)
             {
                 /* It is a truly new medium, remember details for cleanup. */
@@ -1473 +1470 @@
             pMedium.setNull();
             mediaTreeLock.acquire();
+
+            if (FAILED(hrc))
+                break;
         }
 
@@ -4386 +4386 @@
 
 /**
+ * Internal method which returns true if this medium is in the process of being closed.
+ * @return
+ */
+bool Medium::i_isClosing() const
+{
+    return m->fClosing;
+}
+
+/**
  * Internal method to return the medium's full location. Must have caller + locking!
  * @return
@@ -5703 +5712 @@
     }
 
-    // Keep the locks held until after uninit, as otherwise the consistency
-    // of the medium tree cannot be guaranteed.
     uninit();
 

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

@@ -5110 +5110 @@
         strLocationFull = pMedium->i_getLocationFull();
         pParent = pMedium->i_getParent();
+
+        /*
+         * If a separate thread has called Medium::close() for this medium at the same
+         * time as this i_registerMedium() call then there is a window of opportunity in
+         * Medium::i_close() where the media tree lock is dropped before calling
+         * Medium::uninit() (which reacquires the lock) that we can end up here attempting
+         * to register a medium which is in the process of being closed.  In addition, if
+         * this is a differencing medium and Medium::close() is in progress for one its
+         * parent media then we are similarly operating on a media registry in flux.  In
+         * either case registering a medium just before calling Medium::uninit() will
+         * lead to an inconsistent media registry so bail out here since Medium::close()
+         * got to this medium (or one of its parents) first.
+         */
+        if (devType == DeviceType_HardDisk)
+        {
+            ComObjPtr<Medium> pTmpMedium = pMedium;
+            while (pTmpMedium.isNotNull())
+            {
+                AutoCaller mediumAC(pTmpMedium);
+                if (FAILED(mediumAC.hrc())) return mediumAC.hrc();
+                AutoReadLock mlock(pTmpMedium COMMA_LOCKVAL_SRC_POS);
+
+                if (pTmpMedium->i_isClosing())
+                    return setError(E_INVALIDARG,
+                                    tr("Cannot register %s '%s' {%RTuuid} because it is in the process of being closed"),
+                                    pszDevType,
+                                    pTmpMedium->i_getLocationFull().c_str(),
+                                    pTmpMedium->i_getId().raw());
+
+                pTmpMedium = pTmpMedium->i_getParent();
+            }
+        }
     }