[Libreoffice-commits] core.git: Branch 'libreoffice-7-2' - sd/source

[Andrzej Hunt (via logerrit)]

 sd/source/ui/remotecontrol/Transmitter.cxx |   16 +++++++++-------
 sd/source/ui/remotecontrol/Transmitter.hxx |   11 ++++++++---
 2 files changed, 17 insertions(+), 10 deletions(-)

New commits:
commit a4360b155bef02cf3da41e3f05c56d42feef7926
Author:     Andrzej Hunt <andrzej at ahunt.org>
AuthorDate: Sun Jul 11 17:17:27 2021 +0200
Commit:     Michael Stahl <michael.stahl at allotropia.de>
CommitDate: Tue Jul 20 11:22:42 2021 +0200

    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 at 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 at allotropia.de>

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;
 };