From 9b6971095c4d8f24f4ae7055c67a2aac3fc77b63 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 18:31:57 -0500 Subject: [PATCH] fix(daemon): synchronize EmbeddedDaemon fields written across the monitor/start/stop threads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrency audit: last_error_ was written unlocked at the five startProcess() error sites (while the monitor thread could be in setState() writing it under error_mutex_) and read unlocked in start() — a data race on a std::string. process_pid_ / stdout_fd_ (POSIX) and process_handle_ (Windows) are plain scalars written by startProcess, the stop paths and the monitor (reap) and read by accessors. - Route every startProcess() last_error_ write through a new locked setLastError(); read the start() detail via the already-locked getLastError(). - Make process_pid_ / stdout_fd_ / process_handle_ std::atomic (trivially atomic scalars/handles), so individual loads/stores no longer race. Process start/stop ordering and the shutdown hardening are unchanged — this only removes the data races, not the (already-serialized) sequencing. Synchronization only; no fund-path call touched. Threading fix — verified by a clean build + ctest; the race itself is runtime-only. (The Windows process_handle_ atomic is reasoning/compile-only on this Linux box.) Co-Authored-By: Claude Opus 4.8 --- src/daemon/embedded_daemon.cpp | 16 ++++++++-------- src/daemon/embedded_daemon.h | 8 +++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/daemon/embedded_daemon.cpp b/src/daemon/embedded_daemon.cpp index f08ab91..1a3a917 100644 --- a/src/daemon/embedded_daemon.cpp +++ b/src/daemon/embedded_daemon.cpp @@ -703,8 +703,8 @@ bool EmbeddedDaemon::start(const std::string& binary_path, int lockWaitPolls) // ... not executable or wrong architecture"). Surface THAT via setState — which also // stores the Error message into last_error_ — instead of clobbering it with a generic // string that would then be all getLastError()/the UI ever sees. - std::string detail = last_error_.empty() ? std::string("Failed to start dragonxd process") - : last_error_; + std::string detail = getLastError(); + if (detail.empty()) detail = "Failed to start dragonxd process"; DEBUG_LOGF("[ERROR] %s\n", detail.c_str()); setState(State::Error, detail); return false; @@ -822,7 +822,7 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec char errBuf[256] = {0}; FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, err, 0, errBuf, sizeof(errBuf), NULL); - last_error_ = "CreateProcess failed with error " + std::to_string(err) + ": " + errBuf; + setLastError("CreateProcess failed with error " + std::to_string(err) + ": " + errBuf); DEBUG_LOGF("[ERROR] CreateProcess failed: error %lu - %s\nCommand: %s\n", err, errBuf, cmd.c_str()); return false; } @@ -1232,7 +1232,7 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec // Create pipe for stdout/stderr int pipefd[2]; if (pipe(pipefd) == -1) { - last_error_ = "Failed to create pipe: " + std::string(strerror(errno)); + setLastError("Failed to create pipe: " + std::string(strerror(errno))); return false; } @@ -1245,7 +1245,7 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec // macOS, which has no pipe2(). int execpipe[2]; if (pipe(execpipe) == -1) { - last_error_ = "Failed to create exec-status pipe: " + std::string(strerror(errno)); + setLastError("Failed to create exec-status pipe: " + std::string(strerror(errno))); close(pipefd[0]); close(pipefd[1]); return false; @@ -1255,7 +1255,7 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec pid_t pid = fork(); if (pid == -1) { - last_error_ = "Fork failed: " + std::string(strerror(errno)); + setLastError("Fork failed: " + std::string(strerror(errno))); close(pipefd[0]); close(pipefd[1]); close(execpipe[0]); @@ -1370,8 +1370,8 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec close(pipefd[0]); int status; waitpid(pid, &status, 0); - last_error_ = "dragonxd could not be executed: " + std::string(strerror(child_errno)) + - " — not executable or wrong architecture"; + setLastError("dragonxd could not be executed: " + std::string(strerror(child_errno)) + + " — not executable or wrong architecture"); return false; } diff --git a/src/daemon/embedded_daemon.h b/src/daemon/embedded_daemon.h index bfade6a..b5884ec 100644 --- a/src/daemon/embedded_daemon.h +++ b/src/daemon/embedded_daemon.h @@ -504,6 +504,8 @@ private: std::atomic external_daemon_detected_{false}; std::string last_error_; mutable std::mutex error_mutex_; // protects last_error_ (written by main + monitor threads) + // Locked writer for last_error_ (startProcess() runs on the caller thread while the monitor may call setState()). + void setLastError(std::string message) { std::lock_guard lk(error_mutex_); last_error_ = std::move(message); } mutable std::mutex output_mutex_; // protects process_output_ std::string process_output_; size_t output_total_ = 0; // bytes ever appended (the absolute end of process_output_) @@ -511,14 +513,14 @@ private: // Process handle #ifdef _WIN32 - HANDLE process_handle_ = nullptr; + std::atomic process_handle_{nullptr}; // written by start/stop/monitor threads, read by accessors HANDLE stdout_read_ = nullptr; // pipe handle (Linux-style pipe, unused when tailing debug.log) std::string launch_cmd_; // full command line used to launch daemon (for diagnostics) std::string launch_binary_; // binary path used (for diagnostics) std::string launch_workdir_; // working directory used (for diagnostics) #else - pid_t process_pid_ = 0; - int stdout_fd_ = -1; // the child's stdout+stderr (crash diagnostics; the log itself is debug.log) + std::atomic process_pid_{0}; // written by start/stop paths and the monitor (reap), read by accessors + std::atomic stdout_fd_{-1}; // the child's stdout+stderr (crash diagnostics; the log itself is debug.log) void tailDebugLog(); // Waits up to timeout_ms for `pid` to exit, draining its output; true once reaped. Only valid