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

Timestamp:

Jan 31, 2025 11:49:41 PM (3 weeks ago)

Author:

vboxsync

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

167282

Message:

Main/HostDnsService.cpp: Fixed lock order problem with HostDnsMonitorProxy::updateInfo / HostDnsMonitorProxy::pollGlobalExtraData / VirtualBox::getExtraData after r166866. This replaces the IPRT scope-based locking with the main-style locking for flexibility.

Location:

trunk/src/VBox/Main/src-server

Files:

• HostDnsService.cpp (modified) (11 diffs)
• HostDnsService.h (modified) (7 diffs)
• darwin/HostDnsServiceDarwin.cpp (modified) (3 diffs)

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

@@ -95 +95 @@
 }
 
-DECLINLINE(void) detachVectorOfString(const std::vector<std::string>& v, std::vector<com::Utf8Str> &aArray)
+DECLINLINE(void) detachVectorOfStrings(const std::vector<std::string>& v, std::vector<com::Utf8Str> &aArray)
 {
     aArray.resize(v.size());
@@ -247 +247 @@
 }
 
-void HostDnsMonitorProxy::pollGlobalExtraData(void)
-{
+/**
+ * Updates HostDnsMonitorProxy::Data::fLaxComparison every 30 seconds, returning
+ * the new value.
+ *
+ * @note This will leave the lock while calling IVirtualBox::GetExtraData.
+ */
+uint32_t HostDnsMonitorProxy::pollGlobalExtraData(AutoWriteLock &aLock)
+{
+    uint32_t    fLaxComparison = m->fLaxComparison;
     VirtualBox *pVirtualBox = m->pVirtualBox;
-    if (RT_UNLIKELY(pVirtualBox == NULL))
-        return;
-
-    uint64_t uNow = RTTimeNanoTS();
-    if (uNow - m->uLastExtraDataPoll >= RT_NS_30SEC || m->uLastExtraDataPoll == 0)
-    {
-        m->uLastExtraDataPoll = uNow;
-
-        /*
-         * Should we ignore the order of DNS servers?
-         */
-        const com::Bstr bstrHostDNSOrderIgnoreKey("VBoxInternal2/HostDNSOrderIgnore");
-        com::Bstr bstrHostDNSOrderIgnore;
-        pVirtualBox->GetExtraData(bstrHostDNSOrderIgnoreKey.raw(),
-                                 bstrHostDNSOrderIgnore.asOutParam());
-        uint32_t fDNSOrderIgnore = 0;
-        if (bstrHostDNSOrderIgnore.isNotEmpty())
+    if (pVirtualBox)
+    {
+        uint64_t uNow = RTTimeNanoTS();
+        if (uNow - m->uLastExtraDataPoll >= RT_NS_30SEC || m->uLastExtraDataPoll == 0)
         {
-            if (bstrHostDNSOrderIgnore != "0")
+            m->uLastExtraDataPoll = uNow;
+
+            /* We cannot do GetExtraData holding this lock, so temporarily release it. */
+            aLock.release();
+
+            /*
+             * Should we ignore the order of DNS servers?
+             */
+            const com::Bstr bstrHostDNSOrderIgnoreKey("VBoxInternal2/HostDNSOrderIgnore");
+            com::Bstr bstrHostDNSOrderIgnore;
+            pVirtualBox->GetExtraData(bstrHostDNSOrderIgnoreKey.raw(), bstrHostDNSOrderIgnore.asOutParam());
+            uint32_t fDNSOrderIgnore = 0;
+            if (bstrHostDNSOrderIgnore.isNotEmpty() && bstrHostDNSOrderIgnore != "0")
                 fDNSOrderIgnore = HostDnsInformation::IGNORE_SERVER_ORDER;
+
+            if (fDNSOrderIgnore != (fLaxComparison & HostDnsInformation::IGNORE_SERVER_ORDER))
+            {
+                fLaxComparison ^= HostDnsInformation::IGNORE_SERVER_ORDER;
+                LogRel(("HostDnsMonitor: %ls=%ls\n", bstrHostDNSOrderIgnoreKey.raw(), bstrHostDNSOrderIgnore.raw()));
+            }
+
+            /*
+             * Should we ignore changes to the domain name or the search list?
+             */
+            const com::Bstr bstrHostDNSSuffixesIgnoreKey("VBoxInternal2/HostDNSSuffixesIgnore");
+            com::Bstr bstrHostDNSSuffixesIgnore;
+            pVirtualBox->GetExtraData(bstrHostDNSSuffixesIgnoreKey.raw(), bstrHostDNSSuffixesIgnore.asOutParam());
+            uint32_t fDNSSuffixesIgnore = 0;
+            if (bstrHostDNSSuffixesIgnore.isNotEmpty() && bstrHostDNSSuffixesIgnore != "0")
+                fDNSSuffixesIgnore = HostDnsInformation::IGNORE_SUFFIXES;
+
+            if (fDNSSuffixesIgnore != (fLaxComparison & HostDnsInformation::IGNORE_SUFFIXES))
+            {
+                fLaxComparison ^= HostDnsInformation::IGNORE_SUFFIXES;
+                LogRel(("HostDnsMonitor: %ls=%ls\n", bstrHostDNSSuffixesIgnoreKey.raw(), bstrHostDNSSuffixesIgnore.raw()));
+            }
+
+            aLock.acquire();
+            m->fLaxComparison = fLaxComparison;
         }
-
-        if (fDNSOrderIgnore != (m->fLaxComparison & HostDnsInformation::IGNORE_SERVER_ORDER))
-        {
-
-            m->fLaxComparison ^= HostDnsInformation::IGNORE_SERVER_ORDER;
-            LogRel(("HostDnsMonitor: %ls=%ls\n",
-                    bstrHostDNSOrderIgnoreKey.raw(),
-                    bstrHostDNSOrderIgnore.raw()));
-        }
-
-        /*
-         * Should we ignore changes to the domain name or the search list?
-         */
-        const com::Bstr bstrHostDNSSuffixesIgnoreKey("VBoxInternal2/HostDNSSuffixesIgnore");
-        com::Bstr bstrHostDNSSuffixesIgnore;
-        pVirtualBox->GetExtraData(bstrHostDNSSuffixesIgnoreKey.raw(),
-                                 bstrHostDNSSuffixesIgnore.asOutParam());
-        uint32_t fDNSSuffixesIgnore = 0;
-        if (bstrHostDNSSuffixesIgnore.isNotEmpty())
-        {
-            if (bstrHostDNSSuffixesIgnore != "0")
-                fDNSSuffixesIgnore = HostDnsInformation::IGNORE_SUFFIXES;
-        }
-
-        if (fDNSSuffixesIgnore != (m->fLaxComparison & HostDnsInformation::IGNORE_SUFFIXES))
-        {
-
-            m->fLaxComparison ^= HostDnsInformation::IGNORE_SUFFIXES;
-            LogRel(("HostDnsMonitor: %ls=%ls\n",
-                    bstrHostDNSSuffixesIgnoreKey.raw(),
-                    bstrHostDNSSuffixesIgnore.raw()));
-        }
-    }
+    }
+    return fLaxComparison;
 }
 
@@ -324 +324 @@
 HostDnsMonitorProxy::HostDnsMonitorProxy()
     : m(NULL)
+    , m_ObjectLock(LOCKCLASS_OTHEROBJECT, "HostDnsMonitorProxy")
 {
 }
@@ -332 +333 @@
 }
 
-HRESULT HostDnsMonitorProxy::init(VirtualBox* aParent)
+HRESULT HostDnsMonitorProxy::init(VirtualBox *aParent)
 {
     AssertMsgReturn(m == NULL, ("DNS monitor proxy already initialized\n"), E_FAIL);
@@ -363 +364 @@
 }
 
+util::LockHandle *HostDnsMonitorProxy::lockHandle() const
+{
+    return &m_ObjectLock;
+}
+
 void HostDnsMonitorProxy::notify(const HostDnsInformation &info)
 {
@@ -372 +378 @@
 HRESULT HostDnsMonitorProxy::GetNameServers(std::vector<com::Utf8Str> &aNameServers)
 {
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     AssertReturn(m != NULL, E_FAIL);
-    RTCLock grab(m_LockMtx);
 
     LogRel(("HostDnsMonitorProxy::GetNameServers:\n"));
     dumpHostDnsStrVector("name server", m->info.servers);
 
-    detachVectorOfString(m->info.servers, aNameServers);
+    detachVectorOfStrings(m->info.servers, aNameServers);
 
     return S_OK;
@@ -385 +391 @@
 HRESULT HostDnsMonitorProxy::GetDomainName(com::Utf8Str *pDomainName)
 {
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     AssertReturn(m != NULL, E_FAIL);
-    RTCLock grab(m_LockMtx);
-
-    LogRel(("HostDnsMonitorProxy::GetDomainName: %s\n",
-            m->info.domain.empty() ? "no domain set" : m->info.domain.c_str()));
-
+
+    LogRel(("HostDnsMonitorProxy::GetDomainName: %s\n", m->info.domain.empty() ? "no domain set" : m->info.domain.c_str()));
     *pDomainName = m->info.domain.c_str();
 
@@ -398 +402 @@
 HRESULT HostDnsMonitorProxy::GetSearchStrings(std::vector<com::Utf8Str> &aSearchStrings)
 {
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     AssertReturn(m != NULL, E_FAIL);
-    RTCLock grab(m_LockMtx);
 
     LogRel(("HostDnsMonitorProxy::GetSearchStrings:\n"));
     dumpHostDnsStrVector("search string", m->info.searchList);
 
-    detachVectorOfString(m->info.searchList, aSearchStrings);
+    detachVectorOfStrings(m->info.searchList, aSearchStrings);
 
     return S_OK;
@@ -411 +415 @@
 bool HostDnsMonitorProxy::updateInfo(const HostDnsInformation &info)
 {
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     LogRel(("HostDnsMonitor: updating information\n"));
-    RTCLock grab(m_LockMtx);
 
     if (info.equals(m->info))
@@ -420 +424 @@
     }
 
-    pollGlobalExtraData();
+    uint32_t const fLaxComparison = pollGlobalExtraData(alock);
 
     LogRel(("HostDnsMonitor: old information\n"));
@@ -427 +431 @@
     dumpHostDnsInformation(info);
 
-    bool fIgnore = m->fLaxComparison != 0 && info.equals(m->info, m->fLaxComparison);
+    bool fIgnore = fLaxComparison != 0 && info.equals(m->info, fLaxComparison);
     m->info = info;
 

trunk/src/VBox/Main/src-server/HostDnsService.h

@@ -62 +62 @@
 /**
  * Base class for host DNS service implementations.
+ *
  * This class supposed to be a real DNS monitor object as a singleton,
  * so it lifecycle starts and ends together with VBoxSVC.
@@ -118 +119 @@
  */
 class HostDnsMonitorProxy
+    : public Lockable
 {
 public:
@@ -134 +136 @@
     HRESULT GetSearchStrings(std::vector<com::Utf8Str> &aSearchStrings);
 
-private:
-
-    void pollGlobalExtraData(void);
+    LockHandle *lockHandle() const RT_OVERRIDE;
+
+private:
+
+    uint32_t pollGlobalExtraData(AutoWriteLock &aLock);
     bool updateInfo(const HostDnsInformation &info);
 
 private:
-
-    mutable RTCLockMtx m_LockMtx;
-
-    struct Data;
-    Data *m;
+    struct Data;
+    Data *m;
+    /** Object lock object. */
+    mutable util::RWLockHandle m_ObjectLock;
 };
 
@@ -162 +165 @@
 protected:
 
-    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs);
-    int monitorThreadProc(void);
+    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs) RT_OVERRIDE;
+    int monitorThreadProc(void) RT_OVERRIDE;
 
 private:
@@ -187 +190 @@
 protected:
 
-    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs);
-    int monitorThreadProc(void);
+    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs) RT_OVERRIDE;
+    int monitorThreadProc(void) RT_OVERRIDE;
 
 private:
@@ -238 +241 @@
 public:
 
-    virtual HRESULT init(HostDnsMonitorProxy *pProxy) {
+    virtual HRESULT init(HostDnsMonitorProxy *pProxy)
+    {
         return HostDnsServiceResolvConf::init(pProxy, "/etc/resolv.conf");
     }
@@ -258 +262 @@
 protected:
 
-    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs);
-    int monitorThreadProc(void);
+    int monitorThreadShutdown(RTMSINTERVAL uTimeoutMs) RT_OVERRIDE;
+    int monitorThreadProc(void) RT_OVERRIDE;
 
     /** Socket end to write shutdown notification to, so the monitor thread will

trunk/src/VBox/Main/src-server/darwin/HostDnsServiceDarwin.cpp

@@ -115 +115 @@
 
     CFRelease(m->m_SourceStop);
+    m->m_SourceStop = NULL;
     CFRelease(m->m_RunLoopRef);
+    m->m_RunLoopRef = NULL;
     CFRelease(m->m_DnsWatcher);
+    m->m_DnsWatcher = NULL;
     CFRelease(m->m_store);
+    m->m_store = NULL;
 
     RTSemEventDestroy(m->m_evtStop);
+    m->m_evtStop = NIL_RTSEMEVENT;
 }
 
@@ -131 +136 @@
         CFRunLoopStop(m->m_RunLoopRef);
 
+        grab.release(); /* bird 2025-01-31: May deadlock otherwise since hostDnsServiceStoreCallback takes the lock. */
         RTSemEventWait(m->m_evtStop, uTimeoutMs);
     }
@@ -163 +169 @@
 
     /* Trigger initial update. */
-    int vrc = updateInfo();
+    int vrc = updateInfo(); /** @todo r=bird: Not holding the lock here, unlike what hostDnsServiceStoreCallback does... */
     AssertRC(vrc); /* Not fatal in release builds. */  /** @todo r=bird: The function always returns VINF_SUCCESS. */