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@collabora.co.uk>

2 files changed, 64 insertions, 54 deletions

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;