sdremote: fix race condition in Transmitter shutdown to plug leak

We need to acquire the mutex in notifyFinished() to avoid a race between
notifyFinished() and the loop in run(). And we also need to check
mFinishRequested) while holding the mutex to avoid the same race
condition. While we're here, rename the mutex to make it more obvious
that it's not only protecting the queues.

The race can happen because the loop in run() blocks until
mQueuesNotEmpty is set. It also resets mQueuesNotEmpty at the end of
each iteration if the queues are empty. So if someone sets
mQueuesNotEmpty in the middle of an iteration, and the loop later resets
it, the loop won't continue iterating. But we're actually using
mQueuesNotEmpty to indicate either that the queues have data OR that we
might want to finish transmission.

And the problem is that notifyFinished() sets mFinishRequested, followed
by setting mQueuesNotEmpty in an attempt to get the loop to process
mFinishRequested (at which point the loop should finish and return).
But as described above, we can easily get into a situation where the
loop resets mQueuesNotEmpty again (at least if there's no more pending
data in the queues). In this scenario, the loop blocks forever
(we won't receive any new messages after notifyFinished()), causing a
leak of both Transmitter and any resources it's using - e.g. the
StreamSocket.

The easiest way to fix this is to make it clear that the mutex protects
all internal state, followed by using it to protect all internal state.

This issue is not a big deal in normal usage - it's a tiny leak, and
users won't connect enough clients for accumulated leaks to pose
any issues. But it's ugly, and potentially problematic for long-running
tests which could run out of filedescriptors because of the socket leak.

I will rename mQueuesNotEmpty to something more obvious (possibly
mProcessingRequired) in a separate commit.

Change-Id: I2e22087054f3f6a02e05c568b1832ccc5a8b47a3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118751
Reviewed-by: Andrzej Hunt <andrzej@ahunt.org>
Tested-by: Jenkins
(cherry picked from commit 8e6cdb02450a876b5c684eb621e1c9383fb1c428)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118917
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>

2 files changed, 17 insertions, 10 deletions

diff --git a/sd/source/ui/remotecontrol/Transmitter.cxx b/sd/source/ui/remotecontrol/Transmitter.cxx
index 8f3b7d24c184..76cda8feae55 100644
--- a/sd/source/ui/remotecontrol/Transmitter.cxx
+++ b/sd/source/ui/remotecontrol/Transmitter.cxx
@@ -17,8 +17,8 @@ using namespace sd;
 Transmitter::Transmitter( IBluetoothSocket* aSocket )
   : pStreamSocket( aSocket ),
     mQueuesNotEmpty(),
-    mFinishRequested(),
-    mQueueMutex(),
+    mMutex(),
+    mFinishRequested( false ),
     mLowPriority(),
     mHighPriority()
 {
@@ -32,10 +32,11 @@ void SAL_CALL Transmitter::run()
     {
         mQueuesNotEmpty.wait();
 
-        if ( mFinishRequested.check() )
-            return;
+        ::osl::MutexGuard aGuard( mMutex );
 
-        ::osl::MutexGuard aQueueGuard( mQueueMutex );
+        if ( mFinishRequested ) {
+            return;
+        }
         if ( !mHighPriority.empty() )
         {
             OString aMessage( mHighPriority.front() );
@@ -60,7 +61,8 @@ void SAL_CALL Transmitter::run()
 
 void Transmitter::notifyFinished()
 {
-    mFinishRequested.set();
+    ::osl::MutexGuard aGuard( mMutex );
+    mFinishRequested = true;
     mQueuesNotEmpty.set();
 }
 
@@ -70,7 +72,7 @@ Transmitter::~Transmitter()
 
 void Transmitter::addMessage( const OString& aMessage, const Priority aPriority )
 {
-    ::osl::MutexGuard aQueueGuard( mQueueMutex );
+    ::osl::MutexGuard aGuard( mMutex );
     switch ( aPriority )
     {
         case PRIORITY_LOW:
diff --git a/sd/source/ui/remotecontrol/Transmitter.hxx b/sd/source/ui/remotecontrol/Transmitter.hxx
index 8acebfeff7e8..1cd94ea26712 100644
--- a/sd/source/ui/remotecontrol/Transmitter.hxx
+++ b/sd/source/ui/remotecontrol/Transmitter.hxx
@@ -37,11 +37,16 @@ private:
     ::sd::IBluetoothSocket* pStreamSocket;
 
     ::osl::Condition mQueuesNotEmpty;
-    ::osl::Condition mFinishRequested;
-
-    ::osl::Mutex mQueueMutex;
 
+    ::osl::Mutex mMutex;
+    /**
+     * Used to indicate that we're done and the transmitter loop should exit.
+     * All access must be guarded my `mMutex`.
+     */
+    bool mFinishRequested;
+    /// Queue for low priority messages. All access must be guarded my `mMutex`.
     std::queue<OString> mLowPriority;
+    /// Queue for high priority messages. All access must be guarded my `mMutex`.
     std::queue<OString> mHighPriority;
 };