[Libreoffice-commits] core.git: sd/source

2021-07-24 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/inc/RemoteServer.hxx |1 -
 1 file changed, 1 deletion(-)

New commits:
commit 4e511cc7dfbf14b6176ce352db8125c16eefeebf
Author: Andrzej Hunt 
AuthorDate: Sat Jul 24 14:23:10 2021 +0200
Commit: Andrzej Hunt 
CommitDate: Sat Jul 24 16:05:19 2021 +0200

sdremote: remove useless comment

It's been there since the beginning, but doesn't help us much:
  e4239e041468 (Initial checkin of remote control., 2012-07-09)

Change-Id: I57425001cd0b9d0d035916405bd6d94329964b4e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119466
Tested-by: Jenkins
Reviewed-by: Andrzej Hunt 

diff --git a/sd/source/ui/inc/RemoteServer.hxx 
b/sd/source/ui/inc/RemoteServer.hxx
index d5dba8c092f0..e676087899d9 100644
--- a/sd/source/ui/inc/RemoteServer.hxx
+++ b/sd/source/ui/inc/RemoteServer.hxx
@@ -8,7 +8,6 @@
  */
 #pragma once
 
-// SERVER
 #include 
 #include 
 
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

[Libreoffice-commits] core.git: sd/source

2021-07-24 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/remotecontrol/BufferedStreamSocket.cxx |4 
 sd/source/ui/remotecontrol/BufferedStreamSocket.hxx |5 -
 2 files changed, 8 insertions(+), 1 deletion(-)

New commits:
commit 2855f12072023930a15ea852e40d05de4a6be164
Author: Andrzej Hunt 
AuthorDate: Sat Jul 17 09:42:47 2021 +0200
Commit: Andrzej Hunt 
CommitDate: Sat Jul 24 09:51:36 2021 +0200

sdremote: close BufferedStreamSocket on destruction

This follows the pattern in osl::Socket which also cleans up on
destruction if necessary.

By closing on destruction we can avoid leaking the underlying socket or
filedescriptor in the numerous scenarios where we throw away the socket
without bothering to call close(). This isn't a big deal in normal
usage - we don't use many sockets in the first place - but you can
quickly run out of filedescriptors when running sufficiently complex
tests.

We also need to make BufferedStreamSocket final to avoid triggering
loplugin:fragiledestructor - BufferedStreamSocket probably should
have been final anyway, so there's no reason not to do so.

Change-Id: I90c271df4b598a6c2b326fde13543e6b27d7a110
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119093
Tested-by: Jenkins
Reviewed-by: Andrzej Hunt 

diff --git a/sd/source/ui/remotecontrol/BufferedStreamSocket.cxx 
b/sd/source/ui/remotecontrol/BufferedStreamSocket.cxx
index b3a9cc154e37..58adb13ca7bf 100644
--- a/sd/source/ui/remotecontrol/BufferedStreamSocket.cxx
+++ b/sd/source/ui/remotecontrol/BufferedStreamSocket.cxx
@@ -47,6 +47,10 @@ BufferedStreamSocket::BufferedStreamSocket( int aSocket ):
 {
 }
 
+BufferedStreamSocket::~BufferedStreamSocket() {
+close();
+}
+
 void BufferedStreamSocket::getPeerAddr(osl::SocketAddr& rAddr)
 {
 assert ( !usingCSocket );
diff --git a/sd/source/ui/remotecontrol/BufferedStreamSocket.hxx 
b/sd/source/ui/remotecontrol/BufferedStreamSocket.hxx
index 08a81cf002f6..6abf7ec1bf8d 100644
--- a/sd/source/ui/remotecontrol/BufferedStreamSocket.hxx
+++ b/sd/source/ui/remotecontrol/BufferedStreamSocket.hxx
@@ -25,7 +25,7 @@ namespace sd
  * returned to being a StreamSocket wrapper if/when Bluetooth is
  * integrated into osl Sockets.
  */
-class BufferedStreamSocket :
+class BufferedStreamSocket final :
 public IBluetoothSocket,
 private ::osl::StreamSocket
 {
@@ -40,6 +40,9 @@ namespace sd
  */
 explicit BufferedStreamSocket( int aSocket );
 BufferedStreamSocket( const BufferedStreamSocket  );
+
+~BufferedStreamSocket();
+
 /**
  * Blocks until a line is read.
  * Returns whatever the last call of recv returned, i.e. 0 or less
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

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

2021-07-21 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/inc/RemoteServer.hxx |2 
 sd/source/ui/remotecontrol/Server.cxx |  116 ++
 2 files changed, 64 insertions(+), 54 deletions(-)

New commits:
commit 7d93272c6ad0a7209c1e5212484ee44970de850f
Author: Andrzej Hunt 
AuthorDate: Fri Jul 16 18:52:57 2021 +0200
Commit: Noel Grandin 
CommitDate: Wed Jul 21 08:42:33 2021 +0200

sdremote: avoid infinite loop if client disconnects during handshake

The following loop can turn into an infinite loop if the client
disconnects at the wrong point in time, because pSocket->readLine()
returns 0 (indicating disconnection), aLine remains empty (no data
was read), and we just keep looping because we never bothered to check
the return value:
  do
  {
  pSocket->readLine( aLine );
  }
   while ( aLine.getLength() > 0 );

Aside from spinning unnecessarily, this infinite loop also stops the
server from being able to accept any new incoming connections - in
effect this causes a DOS of the server.

Hence we need to start checking readLine's return value, and break out
of this loop - and in fact break of the surrounding outer loop too
(because we discard this connection and want to wait for another
 connection to arrive).

Because of the nested looping it's hard to come up with a clean solution
that only involves changing this loop - we'd probably need to introduce
a temporary to remember that the client has disconnected, and check that
temporary after the do...while - letting us 'continue' the outer loop.

Therefore we first extract the code that handles newly accepted clients
into a separate method, which then lets us simplify the implementation
by returning at those points that we know a client has disappeared. That
unfortunately makes this bug-fix patch a little more noisy than
expected, but it is a refactoring that we'd want to make anyway.

(There are further improvement we can make here, but I'll put those in a
 separate commit to simplify cherry-picking of just this fix. Examples
 include moving to smart pointers instead of new/delete, introducing an
 early return instead of the really long if statement, etc.)

Change-Id: I13c6efa622a1b1de10b72757ea07e5f4660749fb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119083
Tested-by: Jenkins
Reviewed-by: Noel Grandin 
(cherry picked from commit fcc4d8e01360baa9ba0bf20eb5e7a1c9af793a02)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119184

diff --git a/sd/source/ui/inc/RemoteServer.hxx 
b/sd/source/ui/inc/RemoteServer.hxx
index 65bc523ad274..d5dba8c092f0 100644
--- a/sd/source/ui/inc/RemoteServer.hxx
+++ b/sd/source/ui/inc/RemoteServer.hxx
@@ -28,6 +28,7 @@ namespace com::sun::star::uno { template  class Reference;
 
 namespace sd
 {
+class BufferedStreamSocket;
 class Communicator;
 
 struct ClientInfo
@@ -81,6 +82,7 @@ namespace sd
 ::std::vector< std::shared_ptr< ClientInfoInternal > > 
mAvailableClients;
 
 void execute() override;
+void handleAcceptedConnection( BufferedStreamSocket *pSocket ) ;
 };
 }
 
diff --git a/sd/source/ui/remotecontrol/Server.cxx 
b/sd/source/ui/remotecontrol/Server.cxx
index d3f1ac67f904..b1be49b45dce 100644
--- a/sd/source/ui/remotecontrol/Server.cxx
+++ b/sd/source/ui/remotecontrol/Server.cxx
@@ -110,73 +110,81 @@ void RemoteServer::execute()
 return; // Closed, or other issue.
 }
 BufferedStreamSocket *pSocket = new BufferedStreamSocket( aSocket);
-OString aLine;
-if ( pSocket->readLine( aLine)
-&& aLine == "LO_SERVER_CLIENT_PAIR"
-&& pSocket->readLine( aLine ) )
-{
-OString aName( aLine );
+handleAcceptedConnection( pSocket );
+}
+SAL_INFO( "sdremote", "shutting down RemoteServer" );
+spServer = nullptr; // Object is destroyed when Thread::execute() ends.
+}
 
-if ( ! pSocket->readLine( aLine ) )
-{
-delete pSocket;
-continue;
-}
-OString aPin( aLine );
+void RemoteServer::handleAcceptedConnection( BufferedStreamSocket *pSocket )
+{
+OString aLine;
+if ( pSocket->readLine( aLine)
+&& aLine == "LO_SERVER_CLIENT_PAIR"
+&& pSocket->readLine( aLine ) )
+{
+OString aName( aLine );
 
-SocketAddr aClientAddr;
-pSocket->getPeerAddr( aClientAddr );
+if ( ! pSocket->readLine( aLine ) )
+{
+delete pSocket;
+return;
+}
+OString aPin( aLine );
 
-MutexGuard aGuard( sDataMutex );
-std::shared_ptr< ClientInfoInternal > pClient =
-std::make_shared(
-OStringToOUString( aName, RTL_TEXTENCODING_UTF8 ),
-pSocket, OStringToOUString( aPin,

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

2021-07-20 Thread 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 
AuthorDate: Sun Jul 11 17:17:27 2021 +0200
Commit: Michael Stahl 
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 
Tested-by: Jenkins
(cherry picked from commit 8e6cdb02450a876b5c684eb621e1c9383fb1c428)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118917
Reviewed-by: Michael Stahl 

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

[Libreoffice-commits] core.git: sd/source

2021-07-17 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/remotecontrol/Server.cxx |  100 +-
 1 file changed, 51 insertions(+), 49 deletions(-)

New commits:
commit 137744a25b26d86b9be16a107b3bd011f6ab4b07
Author: Andrzej Hunt 
AuthorDate: Fri Jul 16 18:58:10 2021 +0200
Commit: Noel Grandin 
CommitDate: Sat Jul 17 11:50:08 2021 +0200

sdremote: introduce early return to improve handleAcceptedConnection

This should make it easier to understand the handshake sequence.

Change-Id: If06e98cdfe7295ed00efae61815a8696a90e9533
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119085
Tested-by: Jenkins
Reviewed-by: Noel Grandin 

diff --git a/sd/source/ui/remotecontrol/Server.cxx 
b/sd/source/ui/remotecontrol/Server.cxx
index e3576bacd52e..83a80e9916df 100644
--- a/sd/source/ui/remotecontrol/Server.cxx
+++ b/sd/source/ui/remotecontrol/Server.cxx
@@ -112,67 +112,69 @@ void RemoteServer::execute()
 void RemoteServer::handleAcceptedConnection( BufferedStreamSocket *pSocket )
 {
 OString aLine;
-if ( pSocket->readLine( aLine)
-&& aLine == "LO_SERVER_CLIENT_PAIR"
-&& pSocket->readLine( aLine ) )
+if ( ! ( pSocket->readLine( aLine)
+ && aLine == "LO_SERVER_CLIENT_PAIR"
+ && pSocket->readLine( aLine ) ) )
 {
-OString aName( aLine );
+SAL_INFO( "sdremote", "client failed to send LO_SERVER_CLIENT_PAIR, 
ignoring" );
+delete pSocket;
+return;
+}
 
-if ( ! pSocket->readLine( aLine ) )
-{
+OString aName( aLine );
+
+if ( ! pSocket->readLine( aLine ) )
+{
+delete pSocket;
+return;
+}
+OString aPin( aLine );
+
+SocketAddr aClientAddr;
+pSocket->getPeerAddr( aClientAddr );
+
+do
+{
+// Read off any additional non-empty lines
+// We know that we at least have the empty termination line to read.
+if ( ! pSocket->readLine( aLine ) ) {
 delete pSocket;
 return;
 }
-OString aPin( aLine );
+}
+while ( aLine.getLength() > 0 );
 
-SocketAddr aClientAddr;
-pSocket->getPeerAddr( aClientAddr );
+MutexGuard aGuard( sDataMutex );
+std::shared_ptr< ClientInfoInternal > pClient =
+std::make_shared(
+OStringToOUString( aName, RTL_TEXTENCODING_UTF8 ),
+pSocket, OStringToOUString( aPin, RTL_TEXTENCODING_UTF8 ) );
+mAvailableClients.push_back( pClient );
 
-do
+// Check if we already have this server.
+Reference< XNameAccess > const xConfig = 
officecfg::Office::Impress::Misc::AuthorisedRemotes::get();
+const Sequence< OUString > aNames = xConfig->getElementNames();
+for ( const auto& rName : aNames )
+{
+if ( rName == pClient->mName )
 {
-// Read off any additional non-empty lines
-// We know that we at least have the empty termination line to 
read.
-if ( ! pSocket->readLine( aLine ) ) {
-delete pSocket;
+Reference xSetItem( xConfig->getByName(rName), 
UNO_QUERY );
+Any axPin(xSetItem->getByName("PIN"));
+OUString sPin;
+axPin >>= sPin;
+
+if ( sPin == pClient->mPin ) {
+SAL_INFO( "sdremote", "client found on validated list -- 
connecting" );
+connectClient( pClient, sPin );
 return;
 }
 }
-while ( aLine.getLength() > 0 );
-
-MutexGuard aGuard( sDataMutex );
-std::shared_ptr< ClientInfoInternal > pClient =
-std::make_shared(
-OStringToOUString( aName, RTL_TEXTENCODING_UTF8 ),
-pSocket, OStringToOUString( aPin, RTL_TEXTENCODING_UTF8 ) );
-mAvailableClients.push_back( pClient );
-
-// Check if we already have this server.
-Reference< XNameAccess > const xConfig = 
officecfg::Office::Impress::Misc::AuthorisedRemotes::get();
-const Sequence< OUString > aNames = xConfig->getElementNames();
-for ( const auto& rName : aNames )
-{
-if ( rName == pClient->mName )
-{
-Reference xSetItem( xConfig->getByName(rName), 
UNO_QUERY );
-Any axPin(xSetItem->getByName("PIN"));
-OUString sPin;
-axPin >>= sPin;
-
-if ( sPin == pClient->mPin ) {
-SAL_INFO( "sdremote", "client found on validated list -- 
connecting" );
-connectClient( pClient, sPin );
-return;
-}
-}
-}
-// Pin not found so inform the client.
-SAL_INFO( "sdremote", "client not found on validated list" );
-pSocket->write( "LO_SERVER_VALIDATING_PIN\n\n",
-strlen( "LO_SERVER_VALIDATING_PIN\n\n" ) );
-} else {
-SAL_INFO( "sdremote", "client failed to send LO_SERVER_CLIENT_PAIR, 
ignoring" );
-

[Libreoffice-commits] core.git: sd/source

2021-07-17 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/remotecontrol/Server.cxx |   11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

New commits:
commit d6078e02832d13054e6552c6e20277fd1f3e83f6
Author: Andrzej Hunt 
AuthorDate: Fri Jul 16 18:55:19 2021 +0200
Commit: Noel Grandin 
CommitDate: Sat Jul 17 11:49:20 2021 +0200

sdremote: clean up now-unneeded "found" flag

Since ???, this code now lives in a standalone method, allowing
us to return as soon as know that the handshake is complete. This
lets us remove aFound and simplify the code a little.

Change-Id: I51d5281492d2e4280a101e67d606073f4d064c29
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119084
Tested-by: Jenkins
Reviewed-by: Noel Grandin 

diff --git a/sd/source/ui/remotecontrol/Server.cxx 
b/sd/source/ui/remotecontrol/Server.cxx
index 6a1716b22e86..e3576bacd52e 100644
--- a/sd/source/ui/remotecontrol/Server.cxx
+++ b/sd/source/ui/remotecontrol/Server.cxx
@@ -149,7 +149,6 @@ void RemoteServer::handleAcceptedConnection( 
BufferedStreamSocket *pSocket )
 // Check if we already have this server.
 Reference< XNameAccess > const xConfig = 
officecfg::Office::Impress::Misc::AuthorisedRemotes::get();
 const Sequence< OUString > aNames = xConfig->getElementNames();
-bool aFound = false;
 for ( const auto& rName : aNames )
 {
 if ( rName == pClient->mName )
@@ -162,18 +161,14 @@ void RemoteServer::handleAcceptedConnection( 
BufferedStreamSocket *pSocket )
 if ( sPin == pClient->mPin ) {
 SAL_INFO( "sdremote", "client found on validated list -- 
connecting" );
 connectClient( pClient, sPin );
-aFound = true;
-break;
+return;
 }
 }
 }
 // Pin not found so inform the client.
-if ( !aFound )
-{
-SAL_INFO( "sdremote", "client not found on validated list" );
-pSocket->write( "LO_SERVER_VALIDATING_PIN\n\n",
+SAL_INFO( "sdremote", "client not found on validated list" );
+pSocket->write( "LO_SERVER_VALIDATING_PIN\n\n",
 strlen( "LO_SERVER_VALIDATING_PIN\n\n" ) );
-}
 } else {
 SAL_INFO( "sdremote", "client failed to send LO_SERVER_CLIENT_PAIR, 
ignoring" );
 delete pSocket;
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

[Libreoffice-commits] core.git: sd/source

2021-07-17 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/inc/RemoteServer.hxx |2 
 sd/source/ui/remotecontrol/Server.cxx |  116 ++
 2 files changed, 64 insertions(+), 54 deletions(-)

New commits:
commit fcc4d8e01360baa9ba0bf20eb5e7a1c9af793a02
Author: Andrzej Hunt 
AuthorDate: Fri Jul 16 18:52:57 2021 +0200
Commit: Noel Grandin 
CommitDate: Sat Jul 17 11:48:37 2021 +0200

sdremote: avoid infinite loop if client disconnects during handshake

The following loop can turn into an infinite loop if the client
disconnects at the wrong point in time, because pSocket->readLine()
returns 0 (indicating disconnection), aLine remains empty (no data
was read), and we just keep looping because we never bothered to check
the return value:
  do
  {
  pSocket->readLine( aLine );
  }
   while ( aLine.getLength() > 0 );

Aside from spinning unnecessarily, this infinite loop also stops the
server from being able to accept any new incoming connections - in
effect this causes a DOS of the server.

Hence we need to start checking readLine's return value, and break out
of this loop - and in fact break of the surrounding outer loop too
(because we discard this connection and want to wait for another
 connection to arrive).

Because of the nested looping it's hard to come up with a clean solution
that only involves changing this loop - we'd probably need to introduce
a temporary to remember that the client has disconnected, and check that
temporary after the do...while - letting us 'continue' the outer loop.

Therefore we first extract the code that handles newly accepted clients
into a separate method, which then lets us simplify the implementation
by returning at those points that we know a client has disappeared. That
unfortunately makes this bug-fix patch a little more noisy than
expected, but it is a refactoring that we'd want to make anyway.

(There are further improvement we can make here, but I'll put those in a
 separate commit to simplify cherry-picking of just this fix. Examples
 include moving to smart pointers instead of new/delete, introducing an
 early return instead of the really long if statement, etc.)

Change-Id: I13c6efa622a1b1de10b72757ea07e5f4660749fb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119083
Tested-by: Jenkins
Reviewed-by: Noel Grandin 

diff --git a/sd/source/ui/inc/RemoteServer.hxx 
b/sd/source/ui/inc/RemoteServer.hxx
index 65bc523ad274..d5dba8c092f0 100644
--- a/sd/source/ui/inc/RemoteServer.hxx
+++ b/sd/source/ui/inc/RemoteServer.hxx
@@ -28,6 +28,7 @@ namespace com::sun::star::uno { template  class Reference;
 
 namespace sd
 {
+class BufferedStreamSocket;
 class Communicator;
 
 struct ClientInfo
@@ -81,6 +82,7 @@ namespace sd
 ::std::vector< std::shared_ptr< ClientInfoInternal > > 
mAvailableClients;
 
 void execute() override;
+void handleAcceptedConnection( BufferedStreamSocket *pSocket ) ;
 };
 }
 
diff --git a/sd/source/ui/remotecontrol/Server.cxx 
b/sd/source/ui/remotecontrol/Server.cxx
index 36e052dd006f..6a1716b22e86 100644
--- a/sd/source/ui/remotecontrol/Server.cxx
+++ b/sd/source/ui/remotecontrol/Server.cxx
@@ -103,73 +103,81 @@ void RemoteServer::execute()
 return; // Closed, or other issue.
 }
 BufferedStreamSocket *pSocket = new BufferedStreamSocket( aSocket);
-OString aLine;
-if ( pSocket->readLine( aLine)
-&& aLine == "LO_SERVER_CLIENT_PAIR"
-&& pSocket->readLine( aLine ) )
-{
-OString aName( aLine );
+handleAcceptedConnection( pSocket );
+}
+SAL_INFO( "sdremote", "shutting down RemoteServer" );
+spServer = nullptr; // Object is destroyed when Thread::execute() ends.
+}
 
-if ( ! pSocket->readLine( aLine ) )
-{
-delete pSocket;
-continue;
-}
-OString aPin( aLine );
+void RemoteServer::handleAcceptedConnection( BufferedStreamSocket *pSocket )
+{
+OString aLine;
+if ( pSocket->readLine( aLine)
+&& aLine == "LO_SERVER_CLIENT_PAIR"
+&& pSocket->readLine( aLine ) )
+{
+OString aName( aLine );
 
-SocketAddr aClientAddr;
-pSocket->getPeerAddr( aClientAddr );
+if ( ! pSocket->readLine( aLine ) )
+{
+delete pSocket;
+return;
+}
+OString aPin( aLine );
 
-MutexGuard aGuard( sDataMutex );
-std::shared_ptr< ClientInfoInternal > pClient =
-std::make_shared(
-OStringToOUString( aName, RTL_TEXTENCODING_UTF8 ),
-pSocket, OStringToOUString( aPin, RTL_TEXTENCODING_UTF8 ) 
);
-mAvailableClients.push_back( pClient );
+SocketAddr aClientAddr;
+

[Libreoffice-commits] core.git: sd/source

2021-07-17 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/remotecontrol/Server.cxx |7 ---
 1 file changed, 7 deletions(-)

New commits:
commit 34309e232c13017c9d178627c652e5ffdeb3
Author: Andrzej Hunt 
AuthorDate: Sat Jul 17 08:44:22 2021 +0200
Commit: Andrzej Hunt 
CommitDate: Sat Jul 17 11:31:03 2021 +0200

sdremote: remove commented out experimental check

The entire if statement was introduced only to check for experimental
mode in the following commit - the xContext check is seemingly only
needed for null safety:
  79c1b16a96a6 (sdremote: make it possible to have only bluetooth enabled, 
2012-11-27)

Later, the epxerimental mode check was disabled in:
  b9d2671ae11d (merge WiFi support into remote control feature toggle, 
2013-08-01)

Given that the remote is clearly no longer experimental, it's
time to remove the commented code entirely - and I think it's also
safe to remove the xContext check too. (Note: the remote IS still
controlled by the EnableSdRemote option, and IS disabled by default.)

Change-Id: Id118924021d5029b4f15df9cbb3eba28f3b902c0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119091
Tested-by: Jenkins
Reviewed-by: Andrzej Hunt 

diff --git a/sd/source/ui/remotecontrol/Server.cxx 
b/sd/source/ui/remotecontrol/Server.cxx
index d3f1ac67f904..36e052dd006f 100644
--- a/sd/source/ui/remotecontrol/Server.cxx
+++ b/sd/source/ui/remotecontrol/Server.cxx
@@ -78,13 +78,6 @@ RemoteServer::~RemoteServer()
 void RemoteServer::execute()
 {
 SAL_INFO( "sdremote", "RemoteServer::execute called" );
-uno::Reference< uno::XComponentContext > xContext = 
comphelper::getProcessComponentContext();
-if (!xContext.is()/* || 
!officecfg::Office::Common::Misc::ExperimentalMode::get(xContext)*/)
-{
-// SAL_INFO("sdremote", "not in experimental mode, disabling TCP 
server");
-spServer = nullptr;
-return;
-}
 osl::SocketAddr aAddr( "0.0.0.0", PORT );
 if ( !mSocket.bind( aAddr ) )
 {
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

[Libreoffice-commits] core.git: sd/source

2021-07-15 Thread Andrzej Hunt (via logerrit) 
 sd/source/ui/remotecontrol/Transmitter.cxx |   14 +++---
 sd/source/ui/remotecontrol/Transmitter.hxx |2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

New commits:
commit f4ea1e6095ff751cbecf10061c4eff9934ffd520
Author: Andrzej Hunt 
AuthorDate: Sun Jul 11 17:22:54 2021 +0200
Commit: Andrzej Hunt 
CommitDate: Fri Jul 16 07:20:36 2021 +0200

sdremote: Transmitter: s/mQueuesNotEmpty/mProcessingRequired/

Make mProcessingRequired's name a bit more self-explanatory to make it
clear what we're actually using it for. See also a 8e6cdb0 which
fixed a race condition caused by incorrect use of this Condition.

Change-Id: I6ad63dbd5ae8ed767f42ea22e568ac47a4d08642
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118752
Tested-by: Jenkins
Reviewed-by: Andrzej Hunt 

diff --git a/sd/source/ui/remotecontrol/Transmitter.cxx 
b/sd/source/ui/remotecontrol/Transmitter.cxx
index 76cda8feae55..f00f15502a45 100644
--- a/sd/source/ui/remotecontrol/Transmitter.cxx
+++ b/sd/source/ui/remotecontrol/Transmitter.cxx
@@ -16,7 +16,7 @@ using namespace sd;
 
 Transmitter::Transmitter( IBluetoothSocket* aSocket )
   : pStreamSocket( aSocket ),
-mQueuesNotEmpty(),
+mProcessingRequired(),
 mMutex(),
 mFinishRequested( false ),
 mLowPriority(),
@@ -30,7 +30,7 @@ void SAL_CALL Transmitter::run()
 
 while ( true )
 {
-mQueuesNotEmpty.wait();
+mProcessingRequired.wait();
 
 ::osl::MutexGuard aGuard( mMutex );
 
@@ -52,9 +52,9 @@ void SAL_CALL Transmitter::run()
 pStreamSocket->write( aMessage.getStr(), aMessage.getLength() );
 }
 
-if ( mLowPriority.empty() && mHighPriority.empty() )
+if ( mLowPriority.empty() && mHighPriority.empty())
 {
-mQueuesNotEmpty.reset();
+mProcessingRequired.reset();
 }
 }
 }
@@ -63,7 +63,7 @@ void Transmitter::notifyFinished()
 {
 ::osl::MutexGuard aGuard( mMutex );
 mFinishRequested = true;
-mQueuesNotEmpty.set();
+mProcessingRequired.set();
 }
 
 Transmitter::~Transmitter()
@@ -82,9 +82,9 @@ void Transmitter::addMessage( const OString& aMessage, const 
Priority aPriority
 mHighPriority.push( aMessage );
 break;
 }
-if ( !mQueuesNotEmpty.check() )
+if ( !mProcessingRequired.check() )
 {
-mQueuesNotEmpty.set();
+mProcessingRequired.set();
 }
 }
 
diff --git a/sd/source/ui/remotecontrol/Transmitter.hxx 
b/sd/source/ui/remotecontrol/Transmitter.hxx
index 1cd94ea26712..c24f5a5a46fe 100644
--- a/sd/source/ui/remotecontrol/Transmitter.hxx
+++ b/sd/source/ui/remotecontrol/Transmitter.hxx
@@ -36,7 +36,7 @@ private:
 
 ::sd::IBluetoothSocket* pStreamSocket;
 
-::osl::Condition mQueuesNotEmpty;
+::osl::Condition mProcessingRequired;
 
 ::osl::Mutex mMutex;
 /**
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

[Libreoffice-commits] core.git: sd/source

2021-07-14 Thread 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 8e6cdb02450a876b5c684eb621e1c9383fb1c428
Author: Andrzej Hunt 
AuthorDate: Sun Jul 11 17:17:27 2021 +0200
Commit: Andrzej Hunt 
CommitDate: Wed Jul 14 21:02:03 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 
Tested-by: Jenkins

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 mLowPriority;
+/// Queue for high priority messages. All access must be guarded my 
`mMutex`.
 std::queue mHighPriority;
 };

[Libreoffice-commits] core.git: configure.ac

2021-07-04 Thread Andrzej Hunt (via logerrit) 
 configure.ac |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

New commits:
commit e1d3c8438ade045aab4452dd59000d27f810ca91
Author: Andrzej Hunt 
AuthorDate: Sun Jul 4 14:54:43 2021 +0200
Commit: Caolán McNamara 
CommitDate: Sun Jul 4 20:35:03 2021 +0200

Fix LIB_FUZZING_ENGINE not-empty check

Switch to '-z $LIB_FUZZING_ENGINE' to check if LIB_FUZZING_ENGINE is
set. The previous version evaluates to false when LIB_FUZZING_ENGINE is
not set, meaning you would not be warned at configure time when using
-enable_fuzzers without setting LIB_FUZZING_ENGINE.

Original broken version landed in:
44b36a0602b04342566362bce3f6bed7d2b096e4
( https://gerrit.libreoffice.org/c/core/+/111691 )

Change-Id: Ic2174777ebf724e471cd605ba54b245e4e804705
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118372
Tested-by: Jenkins
Reviewed-by: Caolán McNamara 

diff --git a/configure.ac b/configure.ac
index 7a9eaf39b24b..b2ca6f8d4edd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -10637,7 +10637,7 @@ AC_MSG_CHECKING([whether to enable fuzzers])
 if test "$enable_fuzzers" != yes; then
 AC_MSG_RESULT([no])
 else
-if test $LIB_FUZZING_ENGINE == ""; then
+if test -z $LIB_FUZZING_ENGINE; then
   AC_MSG_ERROR(['LIB_FUZZING_ENGINE' must be set when using 
--enable-fuzzers. Examples include '-fsanitize=fuzzer'.])
 fi
 AC_MSG_RESULT([yes])
___
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits

[Libreoffice-commits] core.git: config_host.mk.in configure.ac vcl/Executable_602fuzzer.mk vcl/Executable_bmpfuzzer.mk vcl/Executable_cgmfuzzer.mk vcl/Executable_diffuzzer.mk vcl/Executable_docxfuzzer

2021-02-28 Thread Andrzej Hunt (via logerrit) 
 config_host.mk.in |1 +
 configure.ac  |4 
 vcl/Executable_602fuzzer.mk   |2 +-
 vcl/Executable_bmpfuzzer.mk   |2 +-
 vcl/Executable_cgmfuzzer.mk   |2 +-
 vcl/Executable_diffuzzer.mk   |2 +-
 vcl/Executable_docxfuzzer.mk  |2 +-
 vcl/Executable_dxffuzzer.mk   |2 +-
 vcl/Executable_epsfuzzer.mk   |2 +-
 vcl/Executable_fodpfuzzer.mk  |2 +-
 vcl/Executable_fodsfuzzer.mk  |2 +-
 vcl/Executable_fodtfuzzer.mk  |2 +-
 vcl/Executable_giffuzzer.mk   |2 +-
 vcl/Executable_htmlfuzzer.mk  |2 +-
 vcl/Executable_hwpfuzzer.mk   |2 +-
 vcl/Executable_jpgfuzzer.mk   |2 +-
 vcl/Executable_lwpfuzzer.mk   |2 +-
 vcl/Executable_metfuzzer.mk   |2 +-
 vcl/Executable_mmlfuzzer.mk   |2 +-
 vcl/Executable_mtpfuzzer.mk   |2 +-
 vcl/Executable_olefuzzer.mk   |2 +-
 vcl/Executable_pcdfuzzer.mk   |2 +-
 vcl/Executable_pctfuzzer.mk   |2 +-
 vcl/Executable_pcxfuzzer.mk   |2 +-
 vcl/Executable_pngfuzzer.mk   |2 +-
 vcl/Executable_ppmfuzzer.mk   |2 +-
 vcl/Executable_pptfuzzer.mk   |2 +-
 vcl/Executable_pptxfuzzer.mk  |2 +-
 vcl/Executable_psdfuzzer.mk   |2 +-
 vcl/Executable_qpwfuzzer.mk   |2 +-
 vcl/Executable_rasfuzzer.mk   |2 +-
 vcl/Executable_rtffuzzer.mk   |2 +-
 vcl/Executable_scrtffuzzer.mk |2 +-
 vcl/Executable_sftfuzzer.mk   |2 +-
 vcl/Executable_slkfuzzer.mk   |2 +-
 vcl/Executable_svmfuzzer.mk   |2 +-
 vcl/Executable_tgafuzzer.mk   |2 +-
 vcl/Executable_tiffuzzer.mk   |2 +-
 vcl/Executable_wksfuzzer.mk   |2 +-
 vcl/Executable_wmffuzzer.mk   |2 +-
 vcl/Executable_ww2fuzzer.mk   |2 +-
 vcl/Executable_ww6fuzzer.mk   |2 +-
 vcl/Executable_ww8fuzzer.mk   |2 +-
 vcl/Executable_xbmfuzzer.mk   |2 +-
 vcl/Executable_xlsfuzzer.mk   |2 +-
 vcl/Executable_xlsxfuzzer.mk  |2 +-
 vcl/Executable_xpmfuzzer.mk   |2 +-
 47 files changed, 50 insertions(+), 45 deletions(-)

New commits:
commit 44b36a0602b04342566362bce3f6bed7d2b096e4
Author: Andrzej Hunt 
AuthorDate: Sat Feb 27 14:21:56 2021 +0100
Commit: Caolán McNamara 
CommitDate: Sun Feb 28 19:46:58 2021 +0100

Upgrade fuzzers to LIB_FUZZING_ENGINE

And check that LIB_FUZZING_ENGINE is set during configure.

Because:
1. It's easier to build locally this way (you don't need to build or hack a
   libFuzzingEngine.a - instead you can just specify
   LIB_FUZZING_ENGINE=-fsanitize=fuzzer to produce a valid build).
2. Using -lFuzzingEngine is deprecated [1] for various reasons [2].

The old behaviour can be emulated if desired by setting
LIB_FUZZING_ENGINE=-lFuzzingEngine .

This patch was tested as follows:
- Building LO within oss-fuzz via:
python infra/helper.py build_fuzzers --sanitizer address libreoffice 

python infra/helper.py check_build libreoffice
- Building LO fuzzers standalone via:
export CC="clang-11"
export CXX="clang++-11 -stdlib=libc++"
export CFLAGS="-fsanitize=address -fsanitize-address-use-after-scope 
-fsanitize=fuzzer-no-link -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"
export CXXFLAGS="$CFLAGS -stdlib=libc++"
export LDFLAGS="$CFLAGS -Wl,--compress-debug-sections,zlib -lpthread"
export LIB_FUZZING_ENGINE=-fsanitize=fuzzer
./autogen.sh --with-distro=LibreOfficeOssFuzz  --with-system-libxml
make fuzzers
  (--with-system-libxml only appears to be needed because of issues
  specific to my build environment/Suse 15.2. I'm invoking clang-11 simply
  because that's the most modern clang I have installed, plain clang should
  also work on most sufficiently modern systems).

[1]

https://github.com/google/oss-fuzz/blob/481280c65048fd12fb2141b9225af511a9ef7ed2/infra/presubmit.py#L46
[2] https://github.com/google/oss-fuzz/issues/2164

Change-Id: Iddb577c30a39620e72372ef6c2d3fda67f8aabdf
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111691
Tested-by: Jenkins
Tested-by: Caolán McNamara 
Reviewed-by: Caolán McNamara 

diff --git a/config_host.mk.in b/config_host.mk.in
index c6c9b7eae351..2f52785b840a 100644
--- a/config_host.mk.in
+++ b/config_host.mk.in
@@ -353,6 +353,7 @@ export LIBLAYOUT_JAR=@LIBLAYOUT_JAR@
 export LIBLOADER_JAR=@LIBLOADER_JAR@
 export LIBNUMBERTEXT_CFLAGS=$(gb_SPACE)@LIBNUMBERTEXT_CFLAGS@
 export LIBNUMBERTEXT_LIBS=$(gb_SPACE)@LIBNUMBERTEXT_LIBS@
+export LIB_FUZZING_ENGINE=@LIB_FUZZING_ENGINE@
 export LIBO_BIN_FOLDER=@LIBO_BIN_FOLDER@
 export LIBO_BIN_FOLDER_FOR_BUILD=@LIBO_BIN_FOLDER_FOR_BUILD@
 export LIBO_ETC_FOLDER=@LIBO_ETC_FOLDER@
diff --git a/configure.ac b/configure.ac
index 9e0085370d2b..90b0cf01633b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -10396,11 +10396,15 @@ AC_MSG_CHECKING([whether to enable fuzzers])
 if test "$enable_fuzzers" != yes; then
 AC_MSG_RESULT([no])
 else
+if test $LIB_FUZZING_ENGINE == ""; then
+