[Libreoffice-commits] core.git: sd/source
Andrzej Hunt (via logerrit)
logerrit at kemper.freedesktop.org
Sat Jul 17 09:49:12 UTC 2021
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 <andrzej at ahunt.org>
AuthorDate: Fri Jul 16 18:52:57 2021 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
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 <noel.grandin at collabora.co.uk>
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 interface_type> 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<ClientInfoInternal>(
- OStringToOUString( aName, RTL_TEXTENCODING_UTF8 ),
- pSocket, OStringToOUString( aPin, RTL_TEXTENCODING_UTF8 ) );
- mAvailableClients.push_back( pClient );
+ 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.
- do
- {
- pSocket->readLine( aLine );
+ if ( ! pSocket->readLine( aLine ) ) {
+ delete pSocket;
+ return;
}
- while ( aLine.getLength() > 0 );
+ }
+ while ( aLine.getLength() > 0 );
- // 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 )
+ MutexGuard aGuard( sDataMutex );
+ std::shared_ptr< ClientInfoInternal > pClient =
+ std::make_shared<ClientInfoInternal>(
+ 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();
+ bool aFound = false;
+ for ( const auto& rName : aNames )
+ {
+ if ( rName == pClient->mName )
{
- if ( rName == pClient->mName )
- {
- Reference<XNameAccess> 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 );
- aFound = true;
- break;
- }
+ Reference<XNameAccess> 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 );
+ aFound = true;
+ break;
}
}
- // 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",
- strlen( "LO_SERVER_VALIDATING_PIN\n\n" ) );
- }
- } else {
- SAL_INFO( "sdremote", "client failed to send LO_SERVER_CLIENT_PAIR, ignoring" );
- delete pSocket;
}
+ // 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",
+ strlen( "LO_SERVER_VALIDATING_PIN\n\n" ) );
+ }
+ } else {
+ SAL_INFO( "sdremote", "client failed to send LO_SERVER_CLIENT_PAIR, ignoring" );
+ delete pSocket;
}
- SAL_INFO( "sdremote", "shutting down RemoteServer" );
- spServer = nullptr; // Object is destroyed when Thread::execute() ends.
}
RemoteServer *sd::RemoteServer::spServer = nullptr;
More information about the Libreoffice-commits
mailing list