Changeset 53323 in vbox

Timestamp:

Nov 13, 2014 2:52:50 PM (10 years ago)

Author:

vboxsync

Message:

Main/VirtualBox: take vbox lock before locking dhcp server list (#7500)

Location:

trunk/src/VBox/Main

Files:

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

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

@@ -338 +338 @@
     HRESULT i_registerDHCPServer(DHCPServer *aDHCPServer,
                                  bool aSaveRegistry = true);
-    HRESULT i_unregisterDHCPServer(DHCPServer *aDHCPServer,
-                                   bool aSaveRegistry = true);
+    HRESULT i_unregisterDHCPServer(DHCPServer *aDHCPServer);
     HRESULT i_registerNATNetwork(NATNetwork *aNATNetwork,
                                  bool aSaveRegistry = true);

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

@@ -4785 +4785 @@
     IDHCPServer *aP = aServer;
 
-    HRESULT rc = i_unregisterDHCPServer(static_cast<DHCPServer *>(aP), true);
+    HRESULT rc = i_unregisterDHCPServer(static_cast<DHCPServer *>(aP));
 
     return rc;
@@ -4813 +4813 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
+    // Acquire a lock on the VirtualBox object early to avoid lock order issues
+    // when we call i_saveSettings() later on.
+    AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
     // need it below, in findDHCPServerByNetworkName (reading) and in
     // m->allDHCPServers.addChild, so need to get it here to avoid lock
@@ -4835 +4838 @@
 
     m->allDHCPServers.addChild(aDHCPServer);
+    // we need to release the list lock before we attempt to acquire locks
+    // on other objects in i_saveSettings (see @bugref{7500})
+    alock.release();
 
     if (aSaveSettings)
     {
-        AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
+        // we acquired the lock on 'this' earlier to avoid lock order issues
         rc = i_saveSettings();
-        vboxLock.release();
 
         if (FAILED(rc))
-            i_unregisterDHCPServer(aDHCPServer, false /* aSaveSettings */);
+        {
+            alock.acquire();
+            m->allDHCPServers.removeChild(aDHCPServer);
+        }
     }
 
@@ -4853 +4861 @@
  *
  * @param aDHCPServer   DHCP server object to remove.
- * @param aSaveSettings @c true to save settings to disk (default).
- *
- * When @a aSaveSettings is @c true, this operation may fail because of the
- * failed #saveSettings() method it calls. In this case, the DHCP server
- * will NOT be removed from the settingsi when this method returns.
+ *
+ * This operation may fail because of the failed #saveSettings() method it
+ * calls. In this case, the DHCP server will NOT be removed from the settings
+ * when this method returns.
  *
  * @note Locks this object for writing.
  */
-HRESULT VirtualBox::i_unregisterDHCPServer(DHCPServer *aDHCPServer,
-                                           bool aSaveSettings /*= true*/)
+HRESULT VirtualBox::i_unregisterDHCPServer(DHCPServer *aDHCPServer)
 {
     AssertReturn(aDHCPServer != NULL, E_INVALIDARG);
@@ -4872 +4878 @@
     AssertComRCReturn(dhcpServerCaller.rc(), dhcpServerCaller.rc());
 
+    AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock alock(m->allDHCPServers.getLockHandle() COMMA_LOCKVAL_SRC_POS);
     m->allDHCPServers.removeChild(aDHCPServer);
-
-    HRESULT rc = S_OK;
-
-    if (aSaveSettings)
-    {
-        AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
-        rc = i_saveSettings();
-        vboxLock.release();
-
-        if (FAILED(rc))
-            rc = i_registerDHCPServer(aDHCPServer, false /* aSaveSettings */);
+    // we need to release the list lock before we attempt to acquire locks
+    // on other objects in i_saveSettings (see @bugref{7500})
+    alock.release();
+
+    HRESULT rc = i_saveSettings();
+
+    // undo the changes if we failed to save them
+    if (FAILED(rc))
+    {
+        alock.acquire();
+        m->allDHCPServers.addChild(aDHCPServer);
     }