Changeset 94787 in vbox for trunk/src/VBox/Main/src-server/MediumImpl.cpp

Timestamp:

May 2, 2022 7:47:56 PM (3 years ago)

Author:

vboxsync

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

151178

Message:

Main/Medium: Fix the refcount updating behavior of Medium::uninit. It must not increase the refcount of "this" since this could cause double freeing since the uninit call might have been triggered by the refcount reaching 0. bugref:7717

File:

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

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

@@ -1389 +1389 @@
     llParentsTodo.push_back(NULL);
 
-    while (llSettingsTodo.size() > 0)
+    while (!llSettingsTodo.empty())
     {
         const settings::Medium *current = llSettingsTodo.front();
@@ -1580 +1580 @@
     AutoWriteLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
-    MediaList llMediaTodo;
+    /* Must use a list without refcounting help since "this" might already have
+     * reached 0, and then the refcount must not be increased again since it
+     * would otherwise trigger a double free. For all other list entries this
+     * needs manual refcount updating, to make sure the refcount for children
+     * does not drop to 0 too early. */
+    std::list<Medium *> llMediaTodo;
     llMediaTodo.push_back(this);
 
     while (!llMediaTodo.empty())
     {
-        /* This also guarantees that the refcount doesn't actually drop to 0
-         * again while the uninit is already ongoing. */
-        ComObjPtr<Medium> pMedium = llMediaTodo.front();
+        Medium *pMedium = llMediaTodo.front();
         llMediaTodo.pop_front();
 
@@ -1593 +1596 @@
         AutoUninitSpan autoUninitSpan(pMedium);
         if (autoUninitSpan.uninitDone())
+        {
+            if (pMedium != this)
+                pMedium->Release();
             continue;
+        }
 
         Assert(!pMedium->isWriteLockOnCurrentThread());
@@ -1604 +1611 @@
         pMedium->m->formatObj.setNull();
 
-        if (m->state == MediumState_Deleting)
+        if (pMedium->m->state == MediumState_Deleting)
         {
             /* This medium has been already deleted (directly or as part of a
              * merge).  Reparenting has already been done. */
-            Assert(m->pParent.isNull());
-            Assert(m->llChildren.empty());
+            Assert(pMedium->m->pParent.isNull());
+            Assert(pMedium->m->llChildren.empty());
+            if (pMedium != this)
+                pMedium->Release();
             continue;
         }
@@ -1626 +1635 @@
             Medium *pChild = *it;
             pChild->m->pParent.setNull();
+            pChild->AddRef();
             llMediaTodo.push_back(pChild);
         }
@@ -1633 +1643 @@
 
         unconst(pMedium->m->pVirtualBox) = NULL;
+
+        if (pMedium != this)
+            pMedium->Release();
 
         autoUninitSpan.setSucceeded();
@@ -4277 +4290 @@
     bool fAdd = false;
 
-    while (llMediaTodo.size() > 0)
+    while (!llMediaTodo.empty())
     {
         ComObjPtr<Medium> pMedium = llMediaTodo.front();
@@ -4345 +4358 @@
     bool fRemove = false;
 
-    while (llMediaTodo.size() > 0)
+    while (!llMediaTodo.empty())
     {
         ComObjPtr<Medium> pMedium = llMediaTodo.front();
@@ -4654 +4667 @@
     llMediaTodo.push_back(this);
 
-    while (llMediaTodo.size() > 0)
+    while (!llMediaTodo.empty())
     {
         const Medium *pMedium = llMediaTodo.front();
@@ -5019 +5032 @@
     llSettingsTodo.push_back(&data);
 
-    while (llMediaTodo.size() > 0)
+    while (!llMediaTodo.empty())
     {
         ComObjPtr<Medium> pMedium = llMediaTodo.front();