[Client] Hand off single-instance payloads outside the readyRead signal (#7373)

* [Client] Hand off single-instance payloads outside the readyRead signal

Opening a cockatrice:// link from outside a running client showed the
"Join game ...?" confirmation and then crashed on both answers. The
join reached the server first, so the other players saw the user join
and immediately disconnect.

SingleInstanceManager::handleNewConnection() emitted filesReceived from
inside the forwarding socket's readyRead lambda and only disconnected
afterwards. A joingame link makes the handler show a modal QMessageBox,
which runs a nested event loop. The sending instance exits as soon as
it has the acknowledgment, so the socket was disconnected and
deleteLater'd inside that loop: it died in the middle of its own
readyRead signal, and both this lambda and Qt's signal dispatch kept
using the freed memory.

Disconnect right after the acknowledgment and emit filesReceived
through a queued call, so no handler and no nested event loop ever runs
inside the socket's signal. Guarding the disconnect call alone is not
enough, since Qt still touches the socket after the lambda returns.

Add a regression test that reproduces the nested-event-loop hand-off.

* [Tests] Apply cmake-format to the single-instance test wiring

* [Client] Defer the single-instance socket teardown past the readyRead frame

The forwarded payload is emitted synchronously from inside the forwarding
socket's readyRead, which is what lets the modal "Join game?" confirmation
appear. A handler can spin a nested event loop, and the sending instance
exits as soon as it has the ACK, so the socket is disconnected and
deleteLater'd while that loop runs. Control then returns to this frame with
the socket already destroyed, and the inline disconnectFromServer() touched
freed memory.

Queue the teardown instead and guard it with a QPointer taken before the
handlers run -- building the guard afterwards is itself a use-after-free,
since QPointer dereferences the pointer it is constructed from. The payload
still reaches the handlers synchronously, so the confirmation is unchanged.

The regression test covers both halves: the handlers must run while the
socket is still connected, and the socket must not be torn down until the
readyRead frame has returned.

* Lint.

* [Client] Only delete the single-instance socket outside the readyRead frame

The auto-delete on `disconnected` was the remaining use-after-free. A
handler may spin a nested event loop (the modal "Join game?" box), the
sending instance exits as soon as it has the ACK, and so the peer closes
while that loop runs. deleteLater() issued from inside such a loop is not
deferred to the outer loop: Qt frees the object as soon as the nested loop
unwinds, which is still inside this readyRead emission, so Qt's own signal
dispatch and the handler lambda both keep using a freed socket. When the
notification happens to arrive late the bug hides, which is why this only
reproduced on some platforms.

Drop the auto-delete and let the queued teardown own the socket's lifetime:
it disconnects and then deletes, once the frame has unwound. The QPointer
guard was necessary but not sufficient -- it stopped us dereferencing a
dead pointer, not the socket being freed underneath the frame.

The test waits on the socket's destroyed signal instead of polling, so the
teardown assertion does not depend on platform event timing.

* [Client] Do not queue the single-instance socket teardown

The previous commit moved the deleteLater() off the disconnected signal
and into a queued invocation, believing that "queued" meant "after the
readyRead frame returns". It does not. A queued call is delivered by
whichever event loop is running, and the modal "Join game?" box runs a
nested one -- so the teardown was still executed inside the readyRead
emission, which is exactly the use-after-free it claimed to prevent.

Queueing was never necessary. Because nothing connects deleted/close to
the socket's disconnected signal any more, no nested loop can free it
while the handlers run, and emit filesReceived() is synchronous: once it
returns, the frame has unwound and the socket can be closed and deleted
inline. The QPointer stays, covering the one case that remains -- the
primary shutting down while the confirmation box is open, since
QLocalServer destroys its pending connections.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/single_instance_manager.cpp

@@ -1,6 +1,7 @@
 #include "single_instance_manager.h"
 
 #include <QDir>
+#include <QPointer>
 
 namespace
 {
@@ -155,19 +156,44 @@ void SingleInstanceManager::handleNewConnection()
             socket->write(ACK_MESSAGE);
             socket->flush();
 
-            // Drop the payload from the buffer before handling it: the handlers
+            // Drop the payload from the buffer before handing it off, so a
-            // run synchronously and can spin a nested event loop (e.g. a modal
+            // re-entrant event loop cannot re-parse and re-emit the same files.
-            // dialog) that re-reads this socket, which would re-parse and re-emit
-            // the same files.
             buffer->clear();
             *expectedSize = 0;
 
+            // Take the guard while the socket is still alive: QPointer has to be
+            // built from a live pointer, and by the time the handlers below return
+            // the socket may already be gone.
+            QPointer<QLocalSocket> guardedSocket(socket);
+
+            // Hand the payload over synchronously, exactly as before, so the
+            // "Join game?" confirmation still appears. A handler can spin a nested
+            // event loop (that modal box is one), and the sending instance exits as
+            // soon as it has the ACK above, so while that loop runs the peer closes
+            // and this socket is disconnected.
             emit filesReceived(files);
 
-            socket->disconnectFromServer();
+            // The handlers run synchronously, so returning from the emit means the
+            // readyRead frame has unwound and it is safe to touch the socket again.
+            // This teardown must therefore NOT be queued: a queued call would be
+            // picked up by the nested loop above while the frame is still on the
+            // stack, which is precisely the use-after-free this guards against.
+            // The guard covers the one remaining way the socket can die under us --
+            // the primary being torn down (e.g. quit) while the dialog was open --
+            // since QLocalServer owns and destroys its pending connections.
+            if (guardedSocket) {
+                guardedSocket->disconnectFromServer();
+                guardedSocket->deleteLater();
+            }
             return;
         }
     });
 
-    connect(socket, &QLocalSocket::disconnected, socket, &QLocalSocket::deleteLater);
+    // Deliberately no `disconnected -> deleteLater()` here. The handlers above run
+    // synchronously and may spin a nested event loop, during which the sender exits
+    // and this socket is disconnected. A deleteLater() triggered from there is not
+    // deferred to the outer loop: Qt frees the object as soon as that nested loop
+    // unwinds, which is still inside this readyRead emission, so Qt's own signal
+    // dispatch is left holding a freed socket. The socket is instead closed and
+    // deleted once the handlers have returned, on the code path above.
 }

tests/CMakeLists.txt

@@ -55,6 +55,17 @@ add_executable(
 )
 target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
 
+# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt.
+if(WITH_CLIENT)
+  add_executable(
+    single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp
+                                 single_instance_manager_test.cpp
+  )
+  target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
+
+  add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test)
+endif()
+
 find_package(GTest)
 
 if(NOT GTEST_FOUND)
@@ -98,6 +109,7 @@ if(NOT GTEST_FOUND)
   add_dependencies(latency_tracker_test gtest)
   add_dependencies(metrics_registry_test gtest)
   add_dependencies(loader_local_matching_test gtest)
+  add_dependencies(single_instance_manager_test gtest)
 endif()
 
 include_directories(${GTEST_INCLUDE_DIRS})
@@ -153,6 +165,10 @@ target_link_libraries(
   loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}
 )
 
+if(WITH_CLIENT)
+  target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES})
+endif()
+
 add_subdirectory(card_zone_algorithms)
 add_subdirectory(carddatabase)
 add_subdirectory(deck_list_model)

tests/single_instance_manager_test.cpp

@@ -0,0 +1,134 @@
+#include "single_instance_manager.h"
+
+#include <QCoreApplication>
+#include <QDataStream>
+#include <QEventLoop>
+#include <QLocalServer>
+#include <QLocalSocket>
+#include <QTimer>
+#include <gtest/gtest.h>
+
+namespace
+{
+constexpr int HANDSHAKE_TIMEOUT_MS = 5000;
+constexpr int MODAL_DIALOG_MS = 50;
+
+const QStringList PAYLOAD{QStringLiteral("cockatrice://joingame?hostname=127.0.0.1&port=4747&roomid=0&gameid=1")};
+
+/// Serializes a payload the way SingleInstanceManager::forwardToPrimary() does.
+QByteArray encodePayload(const QStringList &files)
+{
+    QByteArray payload;
+    QDataStream out(&payload, QIODevice::WriteOnly);
+    out << files;
+
+    QByteArray message;
+    QDataStream msgStream(&message, QIODevice::WriteOnly);
+    msgStream << static_cast<quint32>(payload.size());
+    message.append(payload);
+    return message;
+}
+} // namespace
+
+// Regression test for the use-after-free in SingleInstanceManager::handleNewConnection().
+//
+// The forwarded payload is handed to the handlers from inside the forwarding
+// socket's readyRead emission, which is what lets the "Join game?" confirmation
+// appear at all. A handler can spin a nested event loop (a modal QMessageBox),
+// and the sending instance exits as soon as it has the acknowledgment, so the
+// peer closes *while that loop runs*. deleteLater() called from inside such a
+// nested loop does not wait for the outer loop: Qt frees the object as soon as
+// the nested loop unwinds, still inside the readyRead emission, so Qt's own
+// signal dispatch and the handler lambda both end up holding a freed socket.
+// Nothing may therefore delete the socket from the disconnected signal, and the
+// teardown is queued until that frame has unwound.
+TEST(SingleInstanceManagerTest, HandlersRunSynchronouslyAndSocketIsNotTouchedAfterTheyReturn)
+{
+    // Scope the hand-off socket to this test process so a Cockatrice the
+    // developer happens to be running cannot claim it.
+    qputenv("USER", QByteArrayLiteral("cockatrice-test-") + QByteArray::number(QCoreApplication::applicationPid()));
+
+    SingleInstanceManager primary;
+    ASSERT_TRUE(primary.tryRun(QStringList())) << "could not become the primary instance";
+
+    auto *server = primary.findChild<QLocalServer *>();
+    ASSERT_NE(nullptr, server) << "the primary instance is not listening";
+
+    QLocalSocket sender;
+    QStringList handled;
+    QByteArray acknowledgment;
+    int handlerCalls = 0;
+    bool socketConnectedWhenHandled = false;
+    bool socketReleased = false;
+
+    // Waited on after the handlers return: the deferred teardown deletes the
+    // forwarding socket once the readyRead frame has unwound.
+    QEventLoop teardownLoop;
+
+    QEventLoop loop;
+    QObject::connect(&primary, &SingleInstanceManager::filesReceived, &primary, [&](const QStringList &files) {
+        ++handlerCalls;
+
+        // The payload must reach the handlers while the forwarding socket is
+        // still connected: deferring the emit would suppress the modal "Join
+        // game?" confirmation this hand-off exists to show.
+        auto *connection = server->findChild<QLocalSocket *>();
+        socketConnectedWhenHandled = connection != nullptr && connection->state() == QLocalSocket::ConnectedState;
+
+        // Observe the socket's destruction rather than polling for it. `destroyed`
+        // is emitted just before the object goes away, so this stays correct
+        // however the teardown ends up deleting it, and `&primary` keeps the
+        // observer alive even once the socket itself is gone.
+        if (connection != nullptr) {
+            QObject::connect(connection, &QObject::destroyed, &primary, [&] {
+                socketReleased = true;
+                teardownLoop.quit();
+            });
+        }
+
+        // The primary acknowledges the payload before handling it, so that a
+        // sender waiting on the acknowledgment never mistakes a busy primary
+        // for a dead one.
+        sender.waitForReadyRead(HANDSHAKE_TIMEOUT_MS);
+        acknowledgment = sender.readAll();
+
+        // Stand in for the modal "Join game?" confirmation: the sending instance
+        // goes away while this nested loop is running, which is what used to
+        // destroy the socket underneath this readyRead frame.
+        sender.abort();
+
+        QEventLoop modalDialog;
+        QTimer::singleShot(MODAL_DIALOG_MS, &modalDialog, &QEventLoop::quit);
+        modalDialog.exec();
+
+        handled = files;
+        loop.quit();
+    });
+
+    sender.connectToServer(server->serverName());
+    ASSERT_TRUE(sender.waitForConnected(HANDSHAKE_TIMEOUT_MS));
+    sender.write(encodePayload(PAYLOAD));
+    sender.flush();
+
+    QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &loop, &QEventLoop::quit);
+    loop.exec();
+
+    EXPECT_EQ(1, handlerCalls) << "the payload must be handed to the handlers exactly once";
+    EXPECT_EQ(PAYLOAD, handled);
+    EXPECT_TRUE(socketConnectedWhenHandled) << "the payload was not handled from inside the readyRead emission";
+    EXPECT_FALSE(acknowledgment.isEmpty()) << "the primary never acknowledged the payload";
+
+    // The deferred teardown must not be skipped, and must not delete the socket
+    // from inside the nested loop above: either would show up here as a crash or
+    // as a socket that outlives the hand-off and leaks a connection per payload.
+    QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &teardownLoop, &QEventLoop::quit);
+    teardownLoop.exec();
+    EXPECT_TRUE(socketReleased) << "the forwarding socket was never released after the hand-off";
+}
+
+int main(int argc, char **argv)
+{
+    QCoreApplication app(argc, argv);
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}