diff --git a/scripts/test-windows.sh b/scripts/test-windows.sh index 7c14c0c..8bb63a7 100755 --- a/scripts/test-windows.sh +++ b/scripts/test-windows.sh @@ -62,13 +62,16 @@ export DRAGONX_TEST_FIXTURE_DIR="$repo/tests/fixtures" sandbox="$bd/test-appdata" mkdir -p "$sandbox" export APPDATA="$sandbox" LOCALAPPDATA="$sandbox" +# The fake dragonxd.exe plays a "foreign" node (isolated datadir/port) for the never-kill-by-name test. +export DRAGONX_TEST_FAKE_DAEMON="$bd/fake_dragonxd/dragonxd.exe" if grep -qi microsoft /proc/version 2>/dev/null && [[ -e /proc/sys/fs/binfmt_misc/WSLInterop ]]; then # /p: translate each to a Windows path for the Windows process - export WSLENV="${WSLENV:+$WSLENV:}DRAGONX_TEST_FIXTURE_DIR/p:APPDATA/p:LOCALAPPDATA/p" + export WSLENV="${WSLENV:+$WSLENV:}DRAGONX_TEST_FIXTURE_DIR/p:APPDATA/p:LOCALAPPDATA/p:DRAGONX_TEST_FAKE_DAEMON/p" cd "$bd/bin" exec "$exe" elif command -v wine >/dev/null 2>&1; then export DRAGONX_TEST_FIXTURE_DIR="$(winepath -w "$DRAGONX_TEST_FIXTURE_DIR")" + export DRAGONX_TEST_FAKE_DAEMON="$(winepath -w "$DRAGONX_TEST_FAKE_DAEMON")" cd "$bd/bin" exec wine "$exe" else diff --git a/src/app.cpp b/src/app.cpp index ca4463c..a551ca9 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -5391,8 +5391,9 @@ void App::stopEmbeddedDaemon() // Do we hold a live process handle for this node? Capture it BEFORE stop() reaps the pid. If owned, // daemon_controller_->stop() below BLOCKS until the process actually exits. If NOT owned (external / - // adopted / direct-connected), stop() returns at once — we can only ask it over RPC and then watch - // for it to disappear (handled after the stop() call). + // adopted / direct-connected / a lock-gated start that never spawned), stop() returns at once on every + // platform — it only ever waits on (or kills) a process this app launched, never one found by name — + // so we can only ask it over RPC and then watch for it to disappear (handled after the stop() call). const bool owned = daemon_controller_->isRunning(); // Any exit from here on is the stop we are asking for, not a crash — even one that lands before @@ -5677,11 +5678,21 @@ void App::reinstallBundledDaemon() sendStopCommandSafely(*rpc_, "reinstall-daemon"); } runtime.stopDaemonWithPolicy(); - // Wait (bounded, ~12s) for the process to exit and release the binary before overwriting. - for (int i = 0; i < 120 && daemon::EmbeddedDaemon::isRpcPortInUse() - && !ctx.cancelled() && !ctx.shuttingDown(); ++i) { + // Wait (bounded, ~120s) for the old node's PROCESS to exit, not just its RPC port: dragonxd unbinds + // RPC first and can take a minute or more to flush and close its wallet. Until it exits its exe is + // locked on Windows (the overwrite would fail) and it holds the datadir lock (a restart would fail). + auto stillUp = [] { + return daemon::EmbeddedDaemon::isRpcPortInUse() || daemon::EmbeddedDaemon::isDaemonProcessRunning(); + }; + for (int i = 0; i < 1200 && stillUp() && !ctx.cancelled() && !ctx.shuttingDown(); ++i) { ctx.sleepForMs(100); } + if (ctx.cancelled() || ctx.shuttingDown()) return; + if (stillUp()) { + DEBUG_LOGF("[App] Reinstall bundled daemon: aborted — a dragonxd process is still running; " + "binaries left untouched and no new node started\n"); + return; + } // Overwrite the installed dragonx binaries (dragonxd/cli/tx) with the bundled ones. const bool ok = resources::reextractBundledDaemon(); DEBUG_LOGF("[App] Reinstall bundled daemon: %s\n", ok ? "ok" : "FAILED"); diff --git a/src/daemon/embedded_daemon.cpp b/src/daemon/embedded_daemon.cpp index c90bb58..a7a1558 100644 --- a/src/daemon/embedded_daemon.cpp +++ b/src/daemon/embedded_daemon.cpp @@ -51,7 +51,30 @@ EmbeddedDaemon::EmbeddedDaemon() = default; EmbeddedDaemon::~EmbeddedDaemon() { - stop(3000); // Wait up to 3 seconds for clean shutdown + // Never stop() from here: a destructor must not kill a node. On Windows stop() also used to fall + // back to killing ANY dragonxd.exe by name, so destroying an object that had already stopped its own + // process (seed migration's isolated node) could take down the user's main node. Owners call stop() + // explicitly (the app's own daemon is never destroyed: main() ends in _Exit). Just stop watching and + // release what we hold; a process still running is left to finish on its own. + should_stop_ = true; + if (monitor_thread_.joinable()) { + monitor_thread_.join(); + } +#ifdef _WIN32 + if (process_handle_ != nullptr) { + CloseHandle(process_handle_); + process_handle_ = nullptr; + } + if (stdout_read_ != nullptr) { + CloseHandle(stdout_read_); + stdout_read_ = nullptr; + } +#else + if (stdout_fd_ >= 0) { + close(stdout_fd_); + stdout_fd_ = -1; + } +#endif } std::string EmbeddedDaemon::findDaemonBinary() @@ -856,8 +879,9 @@ void EmbeddedDaemon::drainOutput() CloseHandle(hFile); } -void EmbeddedDaemon::stop(int wait_ms) +bool EmbeddedDaemon::stop(int wait_ms) { + stop_requested_ = true; should_stop_ = true; setState(State::Stopping, "Stopping dragonxd..."); @@ -883,6 +907,7 @@ void EmbeddedDaemon::stop(int wait_ms) } }; + bool exitedOnItsOwn = true; if (tracked_alive) { // Our tracked process (dragonxd.exe launched directly) is still alive. // The RPC "stop" was already sent by the caller — wait for it to exit. @@ -891,40 +916,21 @@ void EmbeddedDaemon::stop(int wait_ms) DEBUG_LOGF("Timeout — forcing daemon termination...\n"); TerminateProcess(process_handle_, 1); WaitForSingleObject(process_handle_, 2000); + exitedOnItsOwn = false; } drainOutput(); CloseHandle(process_handle_); process_handle_ = nullptr; } else { - // Tracked handle is dead (batch file case: cmd.exe already exited). - // The real dragonxd.exe may still be running as an orphan. + // No live process of OUR OWN to stop. Never go looking for one by name: findProcessByName returns + // the first dragonxd.exe of any kind (an old node still shutting down, a node on another datadir, + // an isolated seed-migration node), and killing it is how an in-progress shutdown got + // TerminateProcess'd. Nodes we didn't launch only ever get an RPC stop from the caller. if (process_handle_ != nullptr) { CloseHandle(process_handle_); process_handle_ = nullptr; } - - // Find the real dragonxd.exe process by name - DWORD dragonxd_pid = findProcessByName("dragonxd.exe"); - if (dragonxd_pid != 0) { - DEBUG_LOGF("Found orphaned dragonxd.exe (PID %lu) — waiting for RPC stop to take effect...\n", dragonxd_pid); - HANDLE hProc = OpenProcess(PROCESS_TERMINATE | SYNCHRONIZE, FALSE, dragonxd_pid); - if (hProc) { - // RPC stop was already sent — wait for graceful exit - if (!pollWait(hProc, wait_ms)) { - DEBUG_LOGF("Timeout — forcing dragonxd.exe (PID %lu) termination...\n", dragonxd_pid); - TerminateProcess(hProc, 1); - WaitForSingleObject(hProc, 2000); - } - drainOutput(); - CloseHandle(hProc); - DEBUG_LOGF("dragonxd.exe stopped\n"); - } else { - DEBUG_LOGF("Could not open dragonxd.exe process (PID %lu), error %lu\n", - dragonxd_pid, GetLastError()); - } - } else { - DEBUG_LOGF("No dragonxd.exe process found — daemon may have already exited\n"); - } + DEBUG_LOGF("No tracked daemon process to stop (any other dragonxd.exe is not ours and is left alone)\n"); } if (stdout_read_ != nullptr) { @@ -939,6 +945,7 @@ void EmbeddedDaemon::stop(int wait_ms) } setState(State::Stopped, "dragonxd stopped"); + return exitedOnItsOwn; } // Translate Windows NTSTATUS / exit codes to human-readable descriptions @@ -1336,10 +1343,11 @@ bool EmbeddedDaemon::reapWithin(pid_t pid, int timeout_ms, int& status) } } -void EmbeddedDaemon::stop(int wait_ms) +bool EmbeddedDaemon::stop(int wait_ms) { stop_requested_ = true; should_stop_ = true; + bool exitedOnItsOwn = true; // Join the monitor FIRST (it sees should_stop_ within one 100ms tick) so that from here on this is // the only reaper. isRunning() is state-based and stays true until we set Stopped below, so it can't @@ -1371,6 +1379,7 @@ void EmbeddedDaemon::stop(int wait_ms) DEBUG_LOGF("Forcing dragonxd termination with SIGKILL (group -%d)...\n", pid); kill(-pid, SIGKILL); while (waitpid(pid, &status, 0) < 0 && errno == EINTR) {} + exitedOnItsOwn = false; } DEBUG_LOGF("dragonxd exited (%s)\n", describeExitStatus(status).c_str()); } @@ -1384,6 +1393,7 @@ void EmbeddedDaemon::stop(int wait_ms) } setState(State::Stopped, "dragonxd stopped"); + return exitedOnItsOwn; } void EmbeddedDaemon::monitorProcess() @@ -1447,8 +1457,8 @@ bool EmbeddedDaemon::isDaemonProcessRunning() #ifdef _WIN32 return findProcessByName("dragonxd.exe") != 0; #elif defined(__linux__) - // Scan /proc for a process whose comm is exactly "dragonxd". Iterate with an error_code so a proc - // entry vanishing mid-scan (a process exiting) can't throw. + // Scan /proc for a dragonxd process (by comm or by executable: see isDaemonProcessIdentity). Iterate + // with an error_code so a proc entry vanishing mid-scan (a process exiting) can't throw. std::error_code ec; fs::directory_iterator it("/proc", ec), end; for (; !ec && it != end; it.increment(ec)) { @@ -1456,7 +1466,10 @@ bool EmbeddedDaemon::isDaemonProcessRunning() if (pid.empty() || pid[0] < '0' || pid[0] > '9') continue; // numeric pid dirs only std::ifstream f((it->path() / "comm").string()); std::string comm; - if (f && std::getline(f, comm) && comm == "dragonxd") return true; + if (f) std::getline(f, comm); + std::error_code linkEc; // another user's process: EACCES, leaving the name empty + const std::string exe = fs::read_symlink(it->path() / "exe", linkEc).filename().string(); + if (isDaemonProcessIdentity(comm, exe)) return true; } return false; #else diff --git a/src/daemon/embedded_daemon.h b/src/daemon/embedded_daemon.h index 1ced1d7..b00588a 100644 --- a/src/daemon/embedded_daemon.h +++ b/src/daemon/embedded_daemon.h @@ -55,10 +55,12 @@ public: bool start(const std::string& binary_path = ""); /** - * @brief Stop the running daemon + * @brief Stop the daemon process this object launched (never any other dragonxd) * @param wait_ms Maximum milliseconds to wait for graceful shutdown + * @return true if it exited on its own (or there was nothing to stop), false if it had to be + * force-killed — then its wallet/chain files may not have been closed cleanly */ - void stop(int wait_ms = 5000); + bool stop(int wait_ms = 5000); /** * @brief Record that we are about to ask the daemon to stop (e.g. over RPC) @@ -254,6 +256,22 @@ public: */ static bool isDaemonProcessRunning(); + /** + * @brief Does a /proc entry (its comm + its exe link's basename) belong to a dragonxd? (Linux) + * + * comm alone misses a node in its final shutdown phase: Shutdown() renames the main thread to + * "hush-shutoff" while the process still holds the datadir lock. The exe link keeps the binary + * name, with " (deleted)" appended once the binary was replaced underneath it (reinstall/update). + */ + static bool isDaemonProcessIdentity(const std::string& comm, std::string exeName) + { + static const std::string kDeleted = " (deleted)"; + if (exeName.size() > kDeleted.size() && + exeName.compare(exeName.size() - kDeleted.size(), kDeleted.size(), kDeleted) == 0) + exeName.erase(exeName.size() - kDeleted.size()); + return comm == "dragonxd" || exeName == "dragonxd"; + } + /** Decision returned by evaluateDatadirLockGate(): whether start() may spawn now. */ struct StartLockGateDecision { bool proceed = true; // false => bail before spawning diff --git a/src/daemon/seed_wallet_creator.cpp b/src/daemon/seed_wallet_creator.cpp index 504fbb9..f80c85b 100644 --- a/src/daemon/seed_wallet_creator.cpp +++ b/src/daemon/seed_wallet_creator.cpp @@ -17,6 +17,21 @@ namespace daemon { namespace { +// Asks the isolated node to stop over its own RPC (port + throwaway credentials, never the main node's) +// and waits for it to exit. Without the RPC stop it was only ever force-killed, holding the wallet.dat +// it had just created. Returns false if it still had to be force-killed. +bool stopIsolatedNode(EmbeddedDaemon& node, rpc::RPCClient& cli, int port, const std::string& user, + const std::string& pass, int waitMs) +{ + node.markStopRequested(); + // connect() also succeeds against a node still in warmup, which accepts stop too. + if (cli.isConnected() || cli.connect("127.0.0.1", std::to_string(port), user, pass, /*useTls=*/false)) { + try { cli.call("stop", nlohmann::json::array(), 10L); } catch (const std::exception&) {} + } + cli.disconnect(); + return node.stop(waitMs); +} + // Random alphanumeric token for the isolated node's throwaway RPC credentials (libsodium CSPRNG; // sodium_init() has already run at app startup for the chat crypto). std::string randomToken(int n) @@ -113,7 +128,7 @@ SeedWalletResult SeedWalletCreator::create(bool keepDatadir, } if (!ready) { r.error = "The isolated node did not become ready in time."; - temp.stop(20000); + stopIsolatedNode(temp, cli, port, user, pass, 20000); // datadir is thrown away: a forced stop is harmless fs::remove_all(base, ec); return r; } @@ -146,18 +161,24 @@ SeedWalletResult SeedWalletCreator::create(bool keepDatadir, } } + // 7. Stop the isolated node (graceful; it flushes its tiny empty chain quickly). Adopt later copies + // this wallet.dat, so it must have been closed cleanly: a node that had to be force-killed + // fails the creation (nothing has been swept into it yet, so nothing is lost by retrying). + if (!stopIsolatedNode(temp, cli, port, user, pass, 60000) && r.ok) { + r.ok = false; + r.error = "The isolated node had to be force-stopped, so its new wallet can't be trusted. " + "Please try again."; + } + // W1-2: never hand back a live seed on a failure path. If the mnemonic was exported but a - // later step failed (empty address, or z_getnewaddress threw), the caller discards this - // result without wiping it, which would leave the seed resident. Success keeps it deliberately. + // later step failed (empty address, z_getnewaddress threw, or an unclean stop), the caller + // discards this result without wiping it, which would leave the seed resident. Success keeps + // it deliberately. if (!r.ok && !r.seedPhrase.empty()) { sodium_memzero(&r.seedPhrase[0], r.seedPhrase.size()); r.seedPhrase.clear(); } - // 7. Stop the isolated node (graceful; it flushes its tiny empty chain quickly). - cli.disconnect(); - temp.stop(20000); - // 8. Keep the temp wallet for a later sweep/adopt step, or scrub it. tempDatadir is the // migration root `base`; the new wallet.dat lives in /DRAGONX. r.tempDatadir = base; diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index e7b4c67..f83b743 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -1,3 +1,12 @@ +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include +#else +#include +#include +#endif #include "chat/chat_crypto.h" #include "chat/chat_identity.h" #include "chat/chat_service.h" @@ -135,6 +144,11 @@ void removeTree(const fs::path& path) fs::remove_all(path, ec); } +double secondsSince(std::chrono::steady_clock::time_point t0) +{ + return std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); +} + // A named path under the platform temp dir (/tmp on Linux, %TEMP% on Windows, where "/tmp/..." // would mean C:\tmp). fs::path testTmpPath(const std::string& name) @@ -3305,11 +3319,6 @@ std::filesystem::path writeFakeDaemonScript(const std::string& name, const std:: return script; } -double secondsSince(std::chrono::steady_clock::time_point t0) -{ - return std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); -} - // S4: stop() must see a clean exit and return at once. Before the fix the monitor thread exited // without reaping and the state-based isRunning() stayed true, so every POSIX stop waited out the // whole grace period, then sent SIGTERM, then SIGKILL (here ~5 + 10 = 15s). @@ -3384,8 +3393,109 @@ void testPosixStopSigtermIsGraceful() fsn::remove(script, ec); fsn::remove(events, ec); } + +// S3: destroying an EmbeddedDaemon must not stop (let alone kill) the process. The destructor used to +// call stop(3000), which on Windows fell back to killing any dragonxd.exe by name; owners now stop() +// explicitly and the destructor only stops watching. Before the fix this took ~3s and SIGTERMed it. +void testPosixDestructorLeavesProcessRunning() +{ + using dragonx::daemon::EmbeddedDaemon; + namespace fsn = std::filesystem; + const fsn::path pidFile = fsn::temp_directory_path() / "od_fake_daemon_dtor.pid"; + std::error_code ec; + fsn::remove(pidFile, ec); + const auto script = writeFakeDaemonScript("od_fake_daemon_dtor.sh", + "echo $$ > \"$OD_TEST_PIDFILE\"\nsleep 30\n"); + setenv("OD_TEST_PIDFILE", pidFile.string().c_str(), 1); + + pid_t pid = 0; + auto d = std::make_unique(); + d->setSkipPortCheck(true); + EXPECT_TRUE(d->start(script.string())); + for (int i = 0; i < 300 && pid <= 0; ++i) { + std::ifstream in(pidFile); + long v = 0; + if (in >> v) pid = static_cast(v); + else std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + EXPECT_TRUE(pid > 0); + const auto t0 = std::chrono::steady_clock::now(); + d.reset(); + EXPECT_TRUE(secondsSince(t0) < 1.0); + EXPECT_TRUE(pid > 0 && kill(pid, 0) == 0); // still running + if (pid > 0) { + kill(-pid, SIGKILL); // the test's own child (its own process group) + waitpid(pid, nullptr, 0); + } + unsetenv("OD_TEST_PIDFILE"); + fsn::remove(script, ec); + fsn::remove(pidFile, ec); +} #endif // !_WIN32 +#ifdef _WIN32 +// S3: EmbeddedDaemon must never kill a dragonxd.exe it didn't launch. Before the fix, stop() (and the +// destructor's stop(3000)) with no live process of its own looked up "dragonxd.exe" by name and +// TerminateProcess'd the first match: an old node still shutting down, or the user's real node. A fake +// dragonxd.exe (tools/fake_dragonxd) on an isolated datadir and port plays the foreign node. +void testWindowsStopNeverKillsForeignDaemon() +{ + const char* stub = std::getenv("DRAGONX_TEST_FAKE_DAEMON"); + if (!stub || !*stub) { + std::printf("[skip] testWindowsStopNeverKillsForeignDaemon (set DRAGONX_TEST_FAKE_DAEMON; " + "scripts/test-windows.sh does)\n"); + return; + } + const fs::path root = makeTempDir(); + const fs::path dir = root / "DRAGONX"; + fs::create_directories(dir); + writeTestFile(dir / "DRAGONX.conf", "rpcuser=u\nrpcpassword=p\nrpcport=39769\n"); + _putenv_s("FAKE_REBUILD_SECS", "0"); + _putenv_s("FAKE_WARMUP_SECS", "0"); + std::string cmd = "\"" + std::string(stub) + "\" -datadir=\"" + dir.string() + "\""; + STARTUPINFOA si{}; + si.cb = sizeof(si); + PROCESS_INFORMATION pi{}; + const BOOL started = CreateProcessA(nullptr, &cmd[0], nullptr, nullptr, FALSE, CREATE_NO_WINDOW, + nullptr, nullptr, &si, &pi); + _putenv_s("FAKE_REBUILD_SECS", ""); + _putenv_s("FAKE_WARMUP_SECS", ""); + EXPECT_TRUE(started); + if (!started) return; + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + EXPECT_TRUE(WaitForSingleObject(pi.hProcess, 0) == WAIT_TIMEOUT); // the foreign node is up + + { + dragonx::daemon::EmbeddedDaemon d; + const auto t0 = std::chrono::steady_clock::now(); + EXPECT_TRUE(d.stop(2000)); // nothing of its own to stop: returns at once and kills nothing + EXPECT_TRUE(secondsSince(t0) < 1.0); + } // ...nor does the destructor + EXPECT_TRUE(WaitForSingleObject(pi.hProcess, 500) == WAIT_TIMEOUT); // the foreign node survived + + TerminateProcess(pi.hProcess, 0); // the test's own child, by its own handle + WaitForSingleObject(pi.hProcess, 5000); + CloseHandle(pi.hThread); + CloseHandle(pi.hProcess); + removeTree(root); +} +#endif // _WIN32 + +// S3: on Linux a node in its final shutdown phase reads comm "hush-shutoff"; its executable still says +// dragonxd, so it must still count as running (it holds the datadir lock until it exits). +void testDaemonProcessIdentity() +{ + using dragonx::daemon::EmbeddedDaemon; + EXPECT_TRUE(EmbeddedDaemon::isDaemonProcessIdentity("dragonxd", "dragonxd")); + EXPECT_TRUE(EmbeddedDaemon::isDaemonProcessIdentity("hush-shutoff", "dragonxd")); + EXPECT_TRUE(EmbeddedDaemon::isDaemonProcessIdentity("hush-shutoff", "dragonxd (deleted)")); // binary replaced + EXPECT_TRUE(EmbeddedDaemon::isDaemonProcessIdentity("dragonxd", "")); // another user's: no exe link + EXPECT_FALSE(EmbeddedDaemon::isDaemonProcessIdentity("hush-shutoff", "hushd")); // a different Hush daemon + EXPECT_FALSE(EmbeddedDaemon::isDaemonProcessIdentity("hushd", "hushd")); + EXPECT_FALSE(EmbeddedDaemon::isDaemonProcessIdentity("bash", "bash")); + EXPECT_FALSE(EmbeddedDaemon::isDaemonProcessIdentity("dragonx-cli", "dragonx-cli")); +} + void testDatadirLockGate() { using dragonx::daemon::EmbeddedDaemon; @@ -7953,12 +8063,17 @@ int main() testWalletSecurityWorkflowExecutor(); testDaemonShutdownPolicy(); testDatadirLockGate(); + testDaemonProcessIdentity(); +#ifdef _WIN32 + testWindowsStopNeverKillsForeignDaemon(); +#endif #ifndef _WIN32 testExecFailureReported(); testDaemonCrashDetected(); testPosixStopReturnsAfterCleanExit(); testPosixRequestedStopIsNotACrash(); testPosixStopSigtermIsGraceful(); + testPosixDestructorLeavesProcessRunning(); #endif testPlatformEnsureDirectory(); testVerifySaplingParams();