diff --git a/cockatrice/src/single_instance_manager.cpp b/cockatrice/src/single_instance_manager.cpp index eff0e6586..8d363ff34 100644 --- a/cockatrice/src/single_instance_manager.cpp +++ b/cockatrice/src/single_instance_manager.cpp @@ -1,6 +1,7 @@ #include "single_instance_manager.h" #include +#include 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 - // run synchronously and can spin a nested event loop (e.g. a modal - // dialog) that re-reads this socket, which would re-parse and re-emit - // the same files. + // Drop the payload from the buffer before handing it off, so a + // re-entrant event loop cannot 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 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. } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d80995d26..1f9cdb9a3 100644 --- a/tests/CMakeLists.txt +++ b/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) diff --git a/tests/single_instance_manager_test.cpp b/tests/single_instance_manager_test.cpp new file mode 100644 index 000000000..49193850d --- /dev/null +++ b/tests/single_instance_manager_test.cpp @@ -0,0 +1,134 @@ +#include "single_instance_manager.h" + +#include +#include +#include +#include +#include +#include +#include + +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(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(); + 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(); + 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(); +}