Changeset 50545 in vbox for trunk/src/VBox

Timestamp:

Feb 21, 2014 2:50:41 PM (11 years ago)

Author:

vboxsync

Message:

Main/EventSource+VirtualBox: better event source cleanup (for now used only in iVirtualBox/VBoxSVC), to prevent sabotage by passive event listener calls hanging around

Location:

trunk/src/VBox/Main

Files:

• src-all/EventImpl.cpp (modified) (12 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (1 diff)

trunk/src/VBox/Main/src-all/EventImpl.cpp

@@ -595 +595 @@
     HRESULT dequeue(IEvent **aEvent, LONG aTimeout, AutoLockBase &aAlock);
     HRESULT eventProcessed(IEvent *aEvent, PendingEventsMap::iterator &pit);
+    void shutdown();
 
     void addRef()
@@ -676 +677 @@
 struct EventSource::Data
 {
-    Data()
+    Data() : fShutdown(false)
     {}
 
@@ -682 +683 @@
     EventMap                      mEvMap;
     PendingEventsMap              mPendingMap;
+    bool                          fShutdown;
 };
 
@@ -790 +792 @@
 
         ::RTCritSectDelete(&mcsQLock);
-        ::RTSemEventDestroy(mQEvent);
-    }
+    }
+    shutdown();
 }
 
@@ -910 +912 @@
 }
 
+void ListenerRecord::shutdown()
+{
+    if (mQEvent != NIL_RTSEMEVENT)
+    {
+        RTSEMEVENT tmp = mQEvent;
+        mQEvent = NIL_RTSEMEVENT;
+        ::RTSemEventDestroy(tmp);
+    }
+}
+
 EventSource::EventSource()
 {}
@@ -943 +955 @@
 void EventSource::uninit()
 {
+    {
+        // First of all (before even thinking about entering the uninit span):
+        // make sure that all listeners are are shut down (no pending events or
+        // wait calls), because they cannot be alive without the associated
+        // event source. Otherwise API clients which use long-term (or
+        // indefinite) waits will block VBoxSVC termination (just one example)
+        // for a long time or even infinitely long.
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+        if (!m->fShutdown)
+        {
+            m->fShutdown = true;
+            for (Listeners::iterator it = m->mListeners.begin();
+                 it != m->mListeners.end();
+                 ++it)
+            {
+                it->second.obj()->shutdown();
+            }
+        }
+    }
+
     AutoUninitSpan autoUninitSpan(this);
     if (autoUninitSpan.uninitDone())
@@ -964 +996 @@
     {
         AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+        if (m->fShutdown)
+            return setError(VBOX_E_INVALID_OBJECT_STATE,
+                            tr("This event source is already shut down"));
 
         Listeners::const_iterator it = m->mListeners.find(aListener);
@@ -998 +1034 @@
         if (it != m->mListeners.end())
         {
+            it->second.obj()->shutdown();
             m->mListeners.erase(it);
             // destructor removes refs from the event map
@@ -1036 +1073 @@
     do {
         AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+        if (m->fShutdown)
+            return setError(VBOX_E_INVALID_OBJECT_STATE,
+                            tr("This event source is already shut down"));
 
         VBoxEventType_T evType;
@@ -1083 +1124 @@
                 Listeners::iterator lit = m->mListeners.find(record.obj()->mListener);
                 if (lit != m->mListeners.end())
+                {
+                    lit->second.obj()->shutdown();
                     m->mListeners.erase(lit);
+                }
             }
             // anything else to do with cbRc?
@@ -1111 +1155 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (m->fShutdown)
+        return setError(VBOX_E_INVALID_OBJECT_STATE,
+                        tr("This event source is already shut down"));
 
     Listeners::iterator it = m->mListeners.find(aListener);
@@ -1138 +1186 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (m->fShutdown)
+        return setError(VBOX_E_INVALID_OBJECT_STATE,
+                        tr("This event source is already shut down"));
 
     Listeners::iterator it = m->mListeners.find(aListener);

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

@@ -803 +803 @@
     if (m->pEventSource)
     {
-        // we don't perform uninit() as it's possible that some pending event refers to this source
+        // Must uninit the event source here, because it makes no sense that
+        // it survives longer than the base object. If someone gets an event
+        // with such an event source then that's life and it has to be dealt
+        // with appropriately on the API client side.
+        m->pEventSource->uninit();
         unconst(m->pEventSource).setNull();
     }