From d208046234ed73ddc4d70ae2549479afecbd63bb Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 01:58:56 +0300 Subject: [PATCH 1/9] Add the Process abstraction and move mangosd onto it Implemented: src/shared/Process -- Windows service and POSIX daemon behind one interface, with the server's loop passed as a parameter. Implemented: StopRequested/IsPaused/IsRunningInBackground, so the world loop can read the service manager's state. Implemented: mangosd main() split into a dispatcher and Serve(); one code path for foreground, service and daemon. Fixed: m_ServiceStatus was written by the SCM thread and read by the world loop with no synchronisation. Fixed: ServiceMain re-entered main() with a forged argc. Fixed: -s install returned 1 on success, so `install && net start` never ran. Fixed: uninstall of a running service returned success without deleting it. Fixed: the no-console guard covered only Windows; on POSIX the daemon read EOF from /dev/null and CliService shut the world down after a successful start. Fixed: the 10s ready timeout forwarded SIGALRM to a child that had set it to SIG_DFL, killing a server still loading the world. Timeout is now off by default and a failed start is reported through SIGCHLD. Fixed: the CreateService command line was strcat-ed into a MAX_PATH buffer and left unquoted. Co-Authored-By: Claude Opus 5 --- .gitignore | 5 + src/mangosd/Master.cpp | 38 +-- src/mangosd/mangosd.cpp | 324 +++++++++++--------- src/shared/CMakeLists.txt | 23 ++ src/shared/Process/PosixProcess.cpp | 365 +++++++++++++++++++++++ src/shared/Process/Process.h | 180 +++++++++++ src/shared/Process/ProcessCommon.cpp | 102 +++++++ src/shared/Process/WindowsProcess.cpp | 411 ++++++++++++++++++++++++++ 8 files changed, 1283 insertions(+), 165 deletions(-) create mode 100644 src/shared/Process/PosixProcess.cpp create mode 100644 src/shared/Process/Process.h create mode 100644 src/shared/Process/ProcessCommon.cpp create mode 100644 src/shared/Process/WindowsProcess.cpp diff --git a/.gitignore b/.gitignore index 71177226c..8b401fd48 100644 --- a/.gitignore +++ b/.gitignore @@ -105,3 +105,8 @@ docs/superpowers/ # Generated by GenRevision; 02 writes it to the build tree instead. src/shared/revision_data.h + +# Unification staging area at the repo root. The shared sources live in their +# own tree; this checkout must not start tracking a copy of them. Anchored so +# that a "common" directory nested under src/ is unaffected. +/common/ diff --git a/src/mangosd/Master.cpp b/src/mangosd/Master.cpp index 1a6d08029..28419677b 100644 --- a/src/mangosd/Master.cpp +++ b/src/mangosd/Master.cpp @@ -48,12 +48,7 @@ #include #endif -#ifdef _WIN32 -#include "ServiceWin32.h" -extern int m_ServiceStatus; -#else -#include "PosixDaemon.h" -#endif +#include "Process/Process.h" #include #include @@ -242,12 +237,13 @@ void Master::StartServices() 1000 * uint32(sConfig.GetIntDefault("MaxCoreStuckTime", 0))))); // Console last, so its prompt lands after every other start-up line. -#ifdef _WIN32 + // + // Never in the background, on either platform. A Windows service has no + // stdin; a POSIX daemon's stdin is /dev/null, where the first read returns + // end-of-file -- and CliService reads end-of-file as "the operator closed + // the console" and shuts the world down, seconds after a successful start. const bool consoleWanted = sConfig.GetBoolDefault("Console.Enable", true) - && m_ServiceStatus == -1; // no console in service mode -#else - const bool consoleWanted = sConfig.GetBoolDefault("Console.Enable", true); -#endif + && !Process::IsRunningInBackground(); if (consoleWanted) { m_services.push_back(std::unique_ptr( @@ -346,16 +342,20 @@ void Master::WorldLoop() std::chrono::milliseconds(WORLD_SLEEP_CONST - spent)); } -#ifdef _WIN32 - if (m_ServiceStatus == 0) // service stopped + // What the service manager asked for, if there is one. Both are false + // where there is not, so this needs no #ifdef: on POSIX a stop arrives + // as a signal and the handlers in mangosd.cpp already have it. + if (Process::StopRequested()) { World::StopNow(SHUTDOWN_EXIT_CODE); } - while (m_ServiceStatus == 2) // service paused + + // Stalled, not spinning. A stop arriving while paused clears the pause + // flag as well, so this cannot swallow the shutdown. + while (Process::IsPaused()) { std::this_thread::sleep_for(std::chrono::seconds(1)); } -#endif } sLog.outString("World updater stopped."); @@ -396,9 +396,11 @@ int Master::Run() sWorld.SetInitialWorldSettings(); -#ifndef _WIN32 - detachDaemon(); -#endif + // The world is loaded and about to start serving. On POSIX this releases the + // parent that `-s run` left waiting, so the shell prompt comes back only + // once the server is genuinely up; a failure before this point is reported + // as a failed command instead. No-op in the foreground and on Windows. + Process::ReportReady(); // Publish this realm's flags and the client builds it accepts. const uint8 recommendedOrNew = diff --git a/src/mangosd/mangosd.cpp b/src/mangosd/mangosd.cpp index 56da7dc68..f41c59370 100644 --- a/src/mangosd/mangosd.cpp +++ b/src/mangosd/mangosd.cpp @@ -48,13 +48,14 @@ #include #include #if defined(OPENSSL_VERSION_MAJOR) && (OPENSSL_VERSION_MAJOR >= 3) -# include -# include "Auth/OpenSSLProvider.h" +#include +#include "Auth/OpenSSLProvider.h" #endif #include "Platform/Define.h" #include #include +#include #include #include "Database/DatabaseEnv.h" #include "Config/Config.h" @@ -70,27 +71,17 @@ #include "DBCStores.h" #include "MassMailMgr.h" #include "ScriptMgr.h" - +#include "Process/Process.h" #ifdef _WIN32 -#include "ServiceWin32.h" #include "WheatyExceptionReport.h" - -char serviceName[] = "MaNGOS"; // service short name -char serviceLongName[] = "MaNGOS World Service"; // service long name -char serviceDescription[] = "MaNGOS World Service - no description available"; - -int m_ServiceStatus = -1; - -#else -#include "PosixDaemon.h" #endif -DatabaseType WorldDatabase; ///< Accessor to the world database -DatabaseType CharacterDatabase; ///< Accessor to the character database -DatabaseType LoginDatabase; ///< Accessor to the realm/login database +DatabaseType WorldDatabase; ///< Accessor to the world database +DatabaseType CharacterDatabase; ///< Accessor to the character database +DatabaseType LoginDatabase; ///< Accessor to the realm/login database -uint32 realmID = 0; ///< Id of the realm +uint32 realmID = 0; ///< Id of the realm /** * @brief Clear online status for realm accounts on startup @@ -120,15 +111,15 @@ static void on_signal(int s) { switch (s) { - case SIGINT: - World::StopNow(RESTART_EXIT_CODE); - break; - case SIGTERM: + case SIGINT: + World::StopNow(RESTART_EXIT_CODE); + break; + case SIGTERM: #ifdef _WIN32 - case SIGBREAK: + case SIGBREAK: #endif - World::StopNow(SHUTDOWN_EXIT_CODE); - break; + World::StopNow(SHUTDOWN_EXIT_CODE); + break; } signal(s, on_signal); @@ -137,7 +128,7 @@ static void on_signal(int s) /// Define hook for all termination signals static void hook_signals() { - signal(SIGINT, on_signal); + signal(SIGINT, on_signal); signal(SIGTERM, on_signal); #ifdef _WIN32 signal(SIGBREAK, on_signal); @@ -155,23 +146,26 @@ static void unhook_signals() } /// Print out the usage string for this program on the console. -static void usage(const char* prog) +static void usage(const char *prog) { sLog.outString("Usage: \n %s []\n" - " -v, --version print version and exist\n\r" - " -c use config_file as configuration file\n\r" - " -a, --ahbot use config_file as ahbot configuration file\n\r" -#ifdef WIN32 - " Running as service functions:\n\r" - " -s run run as service\n\r" - " -s install install service\n\r" - " -s uninstall uninstall service\n\r" -#else - " Running as daemon functions:\n\r" - " -s run run as daemon\n\r" - " -s stop stop daemon\n\r" -#endif - , prog); + " -v, --version print version and exit\n\r" + " -c use config_file as configuration file\n\r" + " -a, --ahbot use config_file as ahbot configuration file\n\r" + " Running in the background:\n\r" + " -s run run in the background\n\r" + " -s stop stop the background instance\n\r", + prog); + + // Only where there is a service manager to register with. On POSIX the + // functions exist and refuse, because writing systemd units is not the + // server's job -- so they are not offered here either. + if (Process::HasServiceManager()) + { + sLog.outString( + " -s install register as a system service\n\r" + " -s uninstall deregister the system service\n\r"); + } } /// Progress-bar console sink: forward a fully-built bar redraw to the off-thread @@ -179,7 +173,7 @@ static void usage(const char* prog) /// serialized stdout with the log lines and cannot tear against them. Installed /// once the writer thread is running; before that BarGoLink uses its default /// synchronous sink. -static void MangosBarConsoleSink(char const* bytes, size_t len) +static void MangosBarConsoleSink(char const *bytes, size_t len) { sLog.ConsoleEmitRaw(std::string(bytes, len)); } @@ -191,111 +185,23 @@ static void MangosBarProgressSink(int percent) MaNGOS::Console::ConsoleUI::Instance().SetProgress(percent); } -/// Launch the mangos server -int main(int argc, char** argv) +/** + * @brief The server itself: everything from the banner to the last flush. + * + * ===== THIS IS THE PARAMETER ===== + * + * Handed to Process::RunInBackground() for `-s run`, and called directly in the + * foreground. One loop, and the platform decides how it is entered. + * ================================= + * + * The configuration is already loaded when this runs -- the pid file it names is + * what the POSIX fork and `-s stop` need before the server exists. + * + * @param cfg_file the configuration file that was loaded, for the log line. + * @return the process exit code. + */ +static int Serve(char const *cfg_file) { -#ifdef _WIN32 - // Install the exception handler for unhandled exceptions in the main thread - static WheatyExceptionReport exceptionReport; - SetUnhandledExceptionFilter(WheatyExceptionReport::WheatyUnhandledExceptionFilter); -#endif - - ///- Command line parsing - char const* cfg_file = MANGOSD_CONFIG_LOCATION; - - char serviceDaemonMode = '\0'; - - // Walked by hand rather than with ACE_Get_Opt (gone with the rest of ACE) or - // getopt (absent on MSVC). Four options do not justify a dependency. - for (int i = 1; i < argc; ++i) - { - const std::string arg = argv[i]; - const bool hasValue = (i + 1) < argc; - - if (arg == "-v" || arg == "--version") - { - printf("%s\n", GitRevision::GetProjectRevision()); - return 0; - } - else if ((arg == "-c") && hasValue) - { - cfg_file = argv[++i]; - } - else if ((arg == "-a" || arg == "--ahbot") && hasValue) - { - sAuctionBotConfig.SetConfigFileName(argv[++i]); - } - else if (arg == "-s" && hasValue) - { - const std::string mode = argv[++i]; - if (mode == "run") { serviceDaemonMode = 'r'; } -#ifdef _WIN32 - else if (mode == "install") { serviceDaemonMode = 'i'; } - else if (mode == "uninstall") { serviceDaemonMode = 'u'; } -#else - else if (mode == "stop") { serviceDaemonMode = 's'; } -#endif - else - { - sLog.outError("Runtime-Error: -s unsupported argument %s", mode.c_str()); - usage(argv[0]); - Log::WaitBeforeContinueIfNeed(); - return 1; - } - } - else - { - sLog.outError("Runtime-Error: unsupported option %s", arg.c_str()); - usage(argv[0]); - Log::WaitBeforeContinueIfNeed(); - return 1; - } - } - -#ifdef _WIN32 // windows service command need execute before config read - switch (serviceDaemonMode) - { - case 'i': - if (WinServiceInstall()) - { - sLog.outString("Installing service"); - } - return 1; - case 'u': - if (WinServiceUninstall()) - { - sLog.outString("Uninstalling service"); - } - return 1; - case 'r': - WinServiceRun(); - break; - } -#endif - if (!sConfig.SetSource(cfg_file)) - { - // Try current folder as fallback if SYSCONFDIR path fails - if (!sConfig.SetSource(MANGOSD_CONFIG_NAME)) - { - sLog.outError("Could not find configuration file %s.", cfg_file); - Log::WaitBeforeContinueIfNeed(); - return 1; - } - cfg_file = MANGOSD_CONFIG_NAME; - } - -#ifndef _WIN32 - switch (serviceDaemonMode) - { - case 'r': - startDaemon(); - break; - case 's': - stopDaemon(); - break; - } -#endif - sLog.outString("%s [world-daemon]", GitRevision::GetProjectRevision()); sLog.outString("%s", GitRevision::GetFullRevision()); sLog.outString("%s", GitRevision::GetDepElunaFullRevisionStr()); @@ -319,7 +225,6 @@ int main(int argc, char** argv) } #endif - ///- Set progress bars show mode BarGoLink::SetOutputState(sConfig.GetBoolDefault("ShowProgressBars", true)); @@ -353,9 +258,13 @@ int main(int argc, char** argv) // synchronous path and would write straight over the full-screen frame. // "plain" never draws the loading UI. "auto" and "fancy" both ask for it, // and Start() declines on its own when stdout is not a real terminal. + // + // Not in the background: a service has no terminal at all, and the daemon's + // stdout is /dev/null. Start() would decline anyway, but asking it to draw a + // full-screen frame into a null device is not a question worth putting. const std::string consoleStyle = sConfig.GetStringDefault("Console.Style", "auto"); - if (consoleStyle != "plain" && + if (consoleStyle != "plain" && !Process::IsRunningInBackground() && MaNGOS::Console::ConsoleUI::Instance().Start("MaNGOS Zero", "Vanilla 1.12.x")) { MaNGOS::Console::ConsoleUI::Instance().SetHeaderRight( @@ -419,4 +328,125 @@ int main(int argc, char** argv) return runCode; } + +/// Launch the mangos server +int main(int argc, char **argv) +{ +#ifdef _WIN32 + // Install the exception handler for unhandled exceptions in the main thread + static WheatyExceptionReport exceptionReport; + SetUnhandledExceptionFilter(WheatyExceptionReport::WheatyUnhandledExceptionFilter); +#endif + + ///- Command line parsing + char const *cfg_file = MANGOSD_CONFIG_LOCATION; + + Process::ServiceAction action = Process::ServiceAction::None; + + for (int i = 1; i < argc; ++i) + { + const std::string arg = argv[i]; + const bool hasValue = (i + 1) < argc; + + if (arg == "-v" || arg == "--version") + { + printf("%s\n", GitRevision::GetProjectRevision()); + return 0; + } + else if ((arg == "-c") && hasValue) + { + cfg_file = argv[++i]; + } + else if ((arg == "-a" || arg == "--ahbot") && hasValue) + { + sAuctionBotConfig.SetConfigFileName(argv[++i]); + } + else if (arg == "-s" && hasValue) + { + const std::string mode = argv[++i]; + action = Process::ParseServiceAction(mode); + + // Install and uninstall are accepted only where there is a service + // manager behind them. Elsewhere they would reach a function whose + // whole job is to refuse, which is a worse error message than this + // one. + const bool needsManager = + action == Process::ServiceAction::Install || + action == Process::ServiceAction::Uninstall; + + if (action == Process::ServiceAction::None || + (needsManager && !Process::HasServiceManager())) + { + sLog.outError("Runtime-Error: -s unsupported argument %s", mode.c_str()); + usage(argv[0]); + Log::WaitBeforeContinueIfNeed(); + return 1; + } + } + else + { + sLog.outError("Runtime-Error: unsupported option %s", arg.c_str()); + usage(argv[0]); + Log::WaitBeforeContinueIfNeed(); + return 1; + } + } + + Process::Options processOptions; + processOptions.serviceName = "MaNGOS"; + processOptions.serviceDisplayName = "MaNGOS World Service"; + processOptions.serviceDescription = + "MaNGOS World Service - serves a World of Warcraft 1.12.x realm."; + + // Registering with the service manager touches neither the configuration nor + // the databases, so it runs before the config file is looked for -- which is + // also what lets the service be installed on a host that is not configured + // yet. Zero on success, so `mangosd -s install && net start MaNGOS` works. + switch (action) + { + case Process::ServiceAction::Install: + return Process::Install(processOptions) ? 0 : 1; + + case Process::ServiceAction::Uninstall: + return Process::Uninstall(processOptions) ? 0 : 1; + + default: + break; + } + + if (!sConfig.SetSource(cfg_file)) + { + // Try current folder as fallback if SYSCONFDIR path fails + if (!sConfig.SetSource(MANGOSD_CONFIG_NAME)) + { + sLog.outError("Could not find configuration file %s.", cfg_file); + Log::WaitBeforeContinueIfNeed(); + return 1; + } + cfg_file = MANGOSD_CONFIG_NAME; + } + + // Read here rather than inside Serve(): on POSIX the forked child writes this + // file before its parent is allowed to exit, so that `-s run` followed + // immediately by `-s stop` finds a pid to signal. Serve() writes it again + // with the same value, which also covers the foreground case. + processOptions.pidFile = sConfig.GetStringDefault("PidFile", ""); + + if (action == Process::ServiceAction::Stop) + { + return Process::Stop(processOptions) ? 0 : 1; + } + + // The one code path. Foreground calls it here; the background hands it to the + // platform, which calls it from the service thread or the forked child. + const std::function serve = [cfg_file]() + { return Serve(cfg_file); }; + + if (action == Process::ServiceAction::Run) + { + return Process::RunInBackground(processOptions, serve); + } + + return serve(); +} /// @} diff --git a/src/shared/CMakeLists.txt b/src/shared/CMakeLists.txt index a0c375846..38d51ecf5 100644 --- a/src/shared/CMakeLists.txt +++ b/src/shared/CMakeLists.txt @@ -169,6 +169,24 @@ set(SRC_GRP_POLICIES ) source_group("Policies" FILES ${SRC_GRP_POLICIES}) +# Running in the background: one interface, the host picks the implementation. +# The server's own loop is handed over as a parameter, so main() has one code +# path instead of two #ifdef-ed halves that drift. +set(SRC_GRP_PROCESS + Process/Process.h + Process/ProcessCommon.cpp +) +if(WIN32) + list(APPEND SRC_GRP_PROCESS Process/WindowsProcess.cpp) +else() + list(APPEND SRC_GRP_PROCESS Process/PosixProcess.cpp) +endif() +source_group("Process" FILES ${SRC_GRP_PROCESS}) + +# Kept for realmd, which is a separate repository and still includes these +# headers; mangosd builds against Process/ above. shared is a static library, so +# these objects are simply not pulled into a link that does not name them. +# WheatyExceptionReport is unrelated to services and is needed regardless. if(WIN32) set(SRC_GRP_SVC Win/ServiceWin32.cpp @@ -251,6 +269,7 @@ add_library(shared STATIC ${SRC_GRP_THREAD} ${SRC_GRP_PLATFORM} ${SRC_GRP_POLICIES} + ${SRC_GRP_PROCESS} ${SRC_GRP_SVC} ${SRC_GRP_UTILITIES} ${CMAKE_CURRENT_BINARY_DIR/revision_data.h} @@ -286,6 +305,10 @@ target_link_libraries(shared # Winsock + the AcceptEx/GetAcceptExSockaddrs extensions used by IocpServer. $<$:ws2_32> $<$:mswsock> + # The service control manager, for Process/WindowsProcess.cpp. Named + # rather than relied on: MSVC's default library set happens to include + # it, and that is not something to build on. + $<$:advapi32> $<$:${URING_LIBRARY}> PRIVATE mangos_openssl_strict diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp new file mode 100644 index 000000000..bc7466fd2 --- /dev/null +++ b/src/shared/Process/PosixProcess.cpp @@ -0,0 +1,365 @@ +/** + * SPDX-License-Identifier: GPL-3.0-or-later + * + * MaNGOS is a full featured server for World of Warcraft, supporting + * the following clients: 1.12.x, 2.4.3, 3.3.5a, 4.3.4a and 5.4.8 + * + * Copyright (C) 2005-2026 MaNGOS + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * World of Warcraft, and all World of Warcraft or Warcraft art, images, + * and lore are copyrighted by Blizzard Entertainment, Inc. + */ + +#include "Process/Process.h" + +#include "Log/Log.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace Process +{ + +namespace +{ + +/** + * ===== ONLY sig_atomic_t MAY CROSS INTO A HANDLER ===== + * + * These are written by the process and read from a signal handler, and + * volatile sig_atomic_t is the only type the standard allows to be touched + * there. A plain pid_t in their place is undefined behaviour that happens to + * work until it does not. + * ====================================================== + */ +volatile sig_atomic_t g_parentPid = 0; +volatile sig_atomic_t g_childPid = 0; + +/// Read from the world loop's thread, written once in the forked child before +/// the loop exists. Atomic rather than plain bool because those are not the +/// same thread. +std::atomic g_inBackground{false}; + +/** + * @brief Runs in the PARENT, while it waits for the child to come up. + * + * ===== _exit, NEVER exit ===== + * + * This is a signal handler. exit() runs the atexit handlers and flushes stdio; + * arriving there from inside malloc or a FILE lock deadlocks the process at the + * exact moment it is trying to report a failure. getpid, kill and _exit are all + * async-signal-safe, and nothing else here is called. + * ============================= + */ +void HandleStartupSignal(int signal) +{ + if (getpid() != g_parentPid) + { + return; + } + + // The child says it is up. + if (signal == SIGUSR1) + { + _exit(EXIT_SUCCESS); + } + + // ===== THE CHILD DIED BEFORE IT WAS READY ===== + // + // This is what makes a timeout unnecessary. A start-up that fails -- no + // database, a bad configuration, a missing DBC -- ends with the child + // exiting, and that exit is reported here, immediately and however long the + // load was going to take. Nothing to forward: it is already gone. + // ============================================== + if (signal == SIGCHLD) + { + _exit(EXIT_FAILURE); + } + + // Interrupted, terminated, or the wait timed out. Pass it on to the child so + // a half-started server does not survive the command that started it. + if (g_childPid) + { + kill(static_cast(g_childPid), signal); + } + + _exit(EXIT_FAILURE); +} + +bool RedirectStandardStreams() +{ + // The daemon has no terminal. Left attached, the first write to stdout after + // the terminal closes takes the process down with SIGHUP or a write error at + // an arbitrary moment. + return std::freopen("/dev/null", "rt", stdin) + && std::freopen("/dev/null", "wt", stdout) + && std::freopen("/dev/null", "wt", stderr); +} + +bool WritePidFile(const std::string& path) +{ + if (path.empty()) + { + return true; + } + + std::ofstream file(path.c_str(), std::ios::out | std::ios::trunc); + if (!file) + { + return false; + } + + file << getpid() << '\n'; + return bool(file); +} + +/** + * @brief Is @p pid running the same executable as this process? + * + * @return true when it is, AND when the platform cannot say. A false negative + * would refuse a legitimate stop, which is worse than the stale-pid case + * this guards -- so only a definite mismatch refuses. + */ +bool IsSameExecutable(pid_t pid) +{ +#ifdef __linux__ + char mine[PATH_MAX + 1] = {}; + char theirs[PATH_MAX + 1] = {}; + + const ssize_t m = readlink("/proc/self/exe", mine, sizeof(mine) - 1); + if (m <= 0) + { + return true; + } + + char link[64] = {}; + std::snprintf(link, sizeof(link), "/proc/%ld/exe", static_cast(pid)); + + const ssize_t t = readlink(link, theirs, sizeof(theirs) - 1); + if (t <= 0) + { + // No such process, or not ours to look at. kill() answers that better. + return true; + } + + return std::strcmp(mine, theirs) == 0; +#else + // FreeBSD and macOS need sysctl/libproc for this. Not worth the platform code + // until a stale pid file actually bites somewhere other than Linux. + (void)pid; + return true; +#endif +} + +} // namespace + +bool HasServiceManager() +{ + return false; +} + +bool Install(const Options&) +{ + sLog.outError("This platform has no service manager to install into." + " Use the init system: a systemd unit or an rc.d script."); + return false; +} + +bool Uninstall(const Options&) +{ + sLog.outError("This platform has no service manager to uninstall from."); + return false; +} + +int RunInBackground(const Options& options, const std::function& serve) +{ + if (!serve) + { + return EXIT_FAILURE; + } + + g_parentPid = getpid(); + + std::signal(SIGUSR1, HandleStartupSignal); + std::signal(SIGCHLD, HandleStartupSignal); + std::signal(SIGINT, HandleStartupSignal); + std::signal(SIGTERM, HandleStartupSignal); + std::signal(SIGALRM, HandleStartupSignal); + + // Blocked across the fork. g_childPid is assigned only AFTER fork returns, + // so a SIGTERM landing in that window would find it still zero: the handler + // would forward nothing and the parent would leave a child running with no + // one holding its pid. + sigset_t blocked; + sigset_t previous; + sigemptyset(&blocked); + sigaddset(&blocked, SIGINT); + sigaddset(&blocked, SIGTERM); + sigprocmask(SIG_BLOCK, &blocked, &previous); + + const pid_t child = fork(); + g_childPid = child; + + sigprocmask(SIG_SETMASK, &previous, nullptr); + + if (child < 0) + { + sLog.outError("Cannot fork into the background: %s", std::strerror(errno)); + return EXIT_FAILURE; + } + + if (child > 0) + { + // The parent. It waits here until the child reports ready, dies, or -- + // where one was asked for -- the alarm fires; every one of those leaves + // through the handler. + if (options.readyTimeoutSeconds != 0) + { + alarm(options.readyTimeoutSeconds); + } + + // Looped: pause() also returns after a signal whose handler simply + // returned, and a single call would then fall through and report a + // failure that did not happen. + for (;;) + { + pause(); + } + } + + // The child. Its inherited handlers are for the parent's wait, not for it. + // SIGCHLD back to default matters: the server forks nothing, but leaving a + // parent's handler installed is how an inherited disposition surprises the + // next person who adds a helper process. + std::signal(SIGUSR1, SIG_DFL); + std::signal(SIGALRM, SIG_DFL); + std::signal(SIGCHLD, SIG_DFL); + + umask(0); + + if (setsid() < 0) + { + _exit(EXIT_FAILURE); + } + + // Off whatever directory it was started from, so the daemon does not hold a + // mount busy and cannot be unmounted out from under. + if (chdir("/") < 0) + { + _exit(EXIT_FAILURE); + } + + if (!WritePidFile(options.pidFile)) + { + _exit(EXIT_FAILURE); + } + + if (!RedirectStandardStreams()) + { + _exit(EXIT_FAILURE); + } + + g_inBackground.store(true, std::memory_order_release); + + return serve(); +} + +void ReportReady() +{ + if (g_parentPid && g_parentPid != getpid()) + { + kill(static_cast(g_parentPid), SIGUSR1); + + // Once, and only once: a second one would arrive at a parent that has + // already exited and whose pid may since have been reused. + g_parentPid = 0; + } +} + +bool Stop(const Options& options) +{ + if (options.pidFile.empty()) + { + sLog.outError("Cannot stop a background instance: no pid file was configured."); + return false; + } + + std::ifstream file(options.pidFile.c_str()); + if (!file) + { + sLog.outError("Cannot read the pid file %s", options.pidFile.c_str()); + return false; + } + + long pid = 0; + if (!(file >> pid) || pid <= 0) + { + // A pid of zero or below would go to the process GROUP or to every + // process this user owns. Refused rather than sent. + sLog.outError("The pid file %s does not name a process", options.pidFile.c_str()); + return false; + } + + // A pid file outlives the process it names, and pids are reused. Signalling + // whatever now holds that number is how a stale file comes to interrupt an + // unrelated program. Where the check is available, the target must be running + // the same executable as this one. + if (!IsSameExecutable(static_cast(pid))) + { + sLog.outError("The pid file %s names process %ld, which is not this server;" + " refusing to signal it. Remove the stale file.", + options.pidFile.c_str(), pid); + return false; + } + + if (kill(static_cast(pid), SIGINT) < 0) + { + sLog.outError("Cannot stop process %ld: %s", pid, std::strerror(errno)); + return false; + } + + return true; +} + +bool IsRunningInBackground() +{ + return g_inBackground.load(std::memory_order_acquire); +} + +bool StopRequested() +{ + // There is no service manager here. A stop arrives as SIGINT or SIGTERM, + // which the server's own handlers already turn into World::StopNow(). + return false; +} + +bool IsPaused() +{ + return false; +} + +} // namespace Process diff --git a/src/shared/Process/Process.h b/src/shared/Process/Process.h new file mode 100644 index 000000000..02ec29b51 --- /dev/null +++ b/src/shared/Process/Process.h @@ -0,0 +1,180 @@ +/** + * SPDX-License-Identifier: GPL-3.0-or-later + * + * MaNGOS is a full featured server for World of Warcraft, supporting + * the following clients: 1.12.x, 2.4.3, 3.3.5a, 4.3.4a and 5.4.8 + * + * Copyright (C) 2005-2026 MaNGOS + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * World of Warcraft, and all World of Warcraft or Warcraft art, images, + * and lore are copyrighted by Blizzard Entertainment, Inc. + */ + +#ifndef MANGOS_H_PROCESS +#define MANGOS_H_PROCESS + +#include +#include +#include +#include + +/** + * @file Process.h + * @brief Running in the background, on whichever platform this is. + * + * ===== THE SERVER'S LOOP IS A PARAMETER ===== + * + * Windows and POSIX arrive at the background from opposite directions. On + * Windows the service manager starts the process and calls into it; on POSIX + * the process forks itself and tells its own parent when it is up. Written out + * at the call site, that is two `#ifdef`ed halves of main() that drift. + * + * So the server hands its loop over instead, and the platform decides how it + * gets called. One code path in main(), and the difference stays here. + * ============================================ + * + * What is NOT pretended to be common: registering with a service manager. + * Windows has one and the server can install itself into it; on POSIX that is + * systemd's or rc.d's business and writing unit files is not the server's job. + * The functions say so rather than quietly succeeding. + */ + +namespace Process +{ + +/// What `-s ` on the command line asks for. +enum class ServiceAction +{ + None, ///< run in the foreground, the ordinary case + Install, ///< register with the service manager and exit + Uninstall, ///< deregister and exit + Run, ///< go to the background + Stop ///< signal a running background instance to stop +}; + +/// The action @p word names, or None for anything unrecognised. +ServiceAction ParseServiceAction(std::string_view word); + +/// Everything the platform needs. The names are only read where the platform +/// has a service manager to give them to. +struct Options +{ + /// Where the background instance writes its pid, and where Stop reads it. + std::string pidFile; + + /** + * How long the forking parent waits for the child to report ready before it + * gives up, kills the child and reports failure. POSIX only; on Windows the + * service manager keeps its own timeout. + * + * ===== ZERO MEANS WAIT ===== + * + * Zero -- the default -- sets no alarm: the parent waits until the child + * either reports ready or dies, and the death is what reports the failure. + * That is the right setting for a world server, and a wrong non-zero value + * is not a slow start-up, it is a killed one: loading a full world takes + * minutes on a cold database, so a ten-second alarm would fire in the middle + * of it and terminate the server that was still coming up. + * =========================== + */ + std::uint32_t readyTimeoutSeconds = 0; + + std::string serviceName = "MaNGOS"; + std::string serviceDisplayName = "MaNGOS World Service"; + std::string serviceDescription = "MaNGOS World Service - serves a World of" + " Warcraft realm."; +}; + +/// Whether this platform has a service manager to register with. False on +/// POSIX, and that is not a gap: init is not ours to write into. +bool HasServiceManager(); + +/// Register with the service manager. False, with a reason logged, where there +/// is none. +bool Install(const Options& options); + +/// Deregister. False where there is no service manager. +bool Uninstall(const Options& options); + +/** + * @brief Go to the background and run @p serve there. + * + * On Windows this hands control to the service manager, which starts a thread + * that calls @p serve. On POSIX it forks: the child calls @p serve, and the + * parent waits for ReportReady() before exiting successfully -- so a start-up + * that fails before the server is up is reported to whoever ran the command, + * rather than leaving them with a shell prompt and a dead process. + * + * @return what @p serve returned, or a non-zero code when the background start + * itself failed. + */ +int RunInBackground(const Options& options, const std::function& serve); + +/** + * @brief From inside @p serve, once the server is actually serving. + * + * On POSIX this is what releases the waiting parent. Calling it early makes the + * command look successful while the server is still deciding whether it can + * start; never calling it makes every start look like a timeout. + * + * Harmless and ignored on Windows, and harmless in the foreground. + */ +void ReportReady(); + +/// Signal the instance named by `options.pidFile` to stop. False when there is +/// no pid file, it cannot be read, or the process it names is not there. +bool Stop(const Options& options); + +/** + * ===== WHAT THE LOOP HAS TO ASK ===== + * + * The three below are what the world loop needs from the platform, and they are + * the reason this interface is not just "start me in the background". A service + * manager talks to a running server -- stop it, pause it, resume it -- on its + * own thread, and the answers have to reach the loop somehow. They cross a + * thread boundary, so they cross it through these, not through a shared int. + * ==================================== + */ + +/** + * @brief Is the served loop running detached from a terminal? + * + * True inside a Windows service and inside the forked POSIX child; false in the + * foreground. What it gates is the console: a background instance has no stdin, + * and a CLI reader that treats end-of-input as "shut down" turns a successful + * daemon start into an immediate exit. + */ +bool IsRunningInBackground(); + +/** + * @brief Has the service manager asked the server to stop? + * + * Always false where there is no service manager: on POSIX a stop arrives as + * SIGINT or SIGTERM and the server's own signal handlers already have it. + */ +bool StopRequested(); + +/** + * @brief Is the service manager holding the server paused? + * + * The loop is expected to stall while this is true. Always false on POSIX, + * which has no equivalent control. + */ +bool IsPaused(); + +} // namespace Process + +#endif diff --git a/src/shared/Process/ProcessCommon.cpp b/src/shared/Process/ProcessCommon.cpp new file mode 100644 index 000000000..7b5b8d1ef --- /dev/null +++ b/src/shared/Process/ProcessCommon.cpp @@ -0,0 +1,102 @@ +/** + * SPDX-License-Identifier: GPL-3.0-or-later + * + * MaNGOS is a full featured server for World of Warcraft, supporting + * the following clients: 1.12.x, 2.4.3, 3.3.5a, 4.3.4a and 5.4.8 + * + * Copyright (C) 2005-2026 MaNGOS + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * World of Warcraft, and all World of Warcraft or Warcraft art, images, + * and lore are copyrighted by Blizzard Entertainment, Inc. + */ + +#include "Process/Process.h" + +namespace Process +{ + +namespace +{ + +/** + * @brief ASCII case-insensitive equality. + * + * Deliberately ASCII-only and locale-free: these are four fixed keywords typed + * on a command line, and std::tolower under a Turkish locale would stop + * matching "install". The cast through unsigned char is what keeps a negative + * char out of the arithmetic. + */ +bool EqualsIgnoreCaseAscii(std::string_view left, std::string_view right) +{ + if (left.size() != right.size()) + { + return false; + } + + for (std::size_t i = 0; i < left.size(); ++i) + { + unsigned char a = static_cast(left[i]); + unsigned char b = static_cast(right[i]); + + if (a >= 'A' && a <= 'Z') + { + a = static_cast(a - 'A' + 'a'); + } + + if (b >= 'A' && b <= 'Z') + { + b = static_cast(b - 'A' + 'a'); + } + + if (a != b) + { + return false; + } + } + + return true; +} + +} // namespace + +ServiceAction ParseServiceAction(std::string_view word) +{ + // Case-insensitive: these are typed on a command line, and "-s Install" + // failing silently because of the capital is not a lesson worth teaching. + if (EqualsIgnoreCaseAscii(word, "install")) + { + return ServiceAction::Install; + } + + if (EqualsIgnoreCaseAscii(word, "uninstall")) + { + return ServiceAction::Uninstall; + } + + if (EqualsIgnoreCaseAscii(word, "run")) + { + return ServiceAction::Run; + } + + if (EqualsIgnoreCaseAscii(word, "stop")) + { + return ServiceAction::Stop; + } + + return ServiceAction::None; +} + +} // namespace Process diff --git a/src/shared/Process/WindowsProcess.cpp b/src/shared/Process/WindowsProcess.cpp new file mode 100644 index 000000000..8a2f48823 --- /dev/null +++ b/src/shared/Process/WindowsProcess.cpp @@ -0,0 +1,411 @@ +/** + * SPDX-License-Identifier: GPL-3.0-or-later + * + * MaNGOS is a full featured server for World of Warcraft, supporting + * the following clients: 1.12.x, 2.4.3, 3.3.5a, 4.3.4a and 5.4.8 + * + * Copyright (C) 2005-2026 MaNGOS + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * World of Warcraft, and all World of Warcraft or Warcraft art, images, + * and lore are copyrighted by Blizzard Entertainment, Inc. + */ + +#include "Process/Process.h" + +#include "Log/Log.h" + +#include + +#include +#include +#include +#include + +namespace Process +{ + +namespace +{ + +/// What the service manager last asked for. Written by the control handler on +/// the SCM's own thread and read by the world loop on its own, so they are +/// atomic: a plain int across those two threads is a data race whichever way +/// it is read. +std::atomic g_stopRequested{false}; +std::atomic g_paused{false}; +std::atomic g_inBackground{false}; + +SERVICE_STATUS g_status{}; +SERVICE_STATUS_HANDLE g_statusHandle = nullptr; + +/// The server's loop, handed over by RunInBackground. +std::function g_serve; +std::string g_serviceName; +int g_exitCode = EXIT_FAILURE; + +void PublishStatus(DWORD state) +{ + g_status.dwCurrentState = state; + + if (g_statusHandle) + { + SetServiceStatus(g_statusHandle, &g_status); + } +} + +void WINAPI ControlHandler(DWORD control) +{ + switch (control) + { + case SERVICE_CONTROL_INTERROGATE: + break; + + case SERVICE_CONTROL_SHUTDOWN: + case SERVICE_CONTROL_STOP: + // Only recorded. Stopping is the server's to do, and doing it from + // this thread would tear the world down underneath the loop still + // running in it. + g_stopRequested.store(true, std::memory_order_release); + + // A stop while paused would otherwise never be seen: the loop is + // stalled on the pause flag and never reaches the stop check. + g_paused.store(false, std::memory_order_release); + PublishStatus(SERVICE_STOP_PENDING); + return; + + case SERVICE_CONTROL_PAUSE: + g_paused.store(true, std::memory_order_release); + PublishStatus(SERVICE_PAUSED); + return; + + case SERVICE_CONTROL_CONTINUE: + g_paused.store(false, std::memory_order_release); + PublishStatus(SERVICE_RUNNING); + return; + + default: + break; + } + + PublishStatus(g_status.dwCurrentState); +} + +/// The full path of this executable, or empty when it cannot be determined. +std::string ExecutablePath() +{ + std::vector path(MAX_PATH); + + for (;;) + { + const DWORD written = + GetModuleFileNameA(nullptr, path.data(), DWORD(path.size())); + + if (written == 0) + { + return std::string(); + } + + // Truncation is reported by filling the buffer, not by an error, so the + // only way to tell is that the whole buffer came back. Long-path support + // makes this reachable with an ordinary install directory. + if (written < path.size()) + { + return std::string(path.data(), written); + } + + path.resize(path.size() * 2); + } +} + +/// The directory the executable is in, or empty when it cannot be determined. +std::string ExecutableDirectory() +{ + const std::string full = ExecutablePath(); + + if (full.empty()) + { + return std::string(); + } + + const std::size_t slash = full.find_last_of("\\/"); + + // No separator at all is not a directory: returning an empty string rather + // than an empty prefix keeps the caller from chdir-ing to the drive root. + return slash == std::string::npos ? std::string() : full.substr(0, slash); +} + +void WINAPI ServiceMain(DWORD, char**) +{ + g_status = SERVICE_STATUS{}; + g_status.dwServiceType = SERVICE_WIN32_OWN_PROCESS; + g_status.dwCurrentState = SERVICE_START_PENDING; + g_status.dwControlsAccepted = + SERVICE_ACCEPT_STOP | SERVICE_ACCEPT_SHUTDOWN | SERVICE_ACCEPT_PAUSE_CONTINUE; + g_status.dwWin32ExitCode = NO_ERROR; + + g_statusHandle = RegisterServiceCtrlHandlerA(g_serviceName.c_str(), ControlHandler); + + if (!g_statusHandle) + { + return; + } + + PublishStatus(SERVICE_START_PENDING); + + // The service manager starts the process from system32, not from where the + // binary lives, so every relative path in the configuration would resolve + // somewhere else. Both the lookup and the chdir are checked: an empty + // directory handed to SetCurrentDirectory succeeds at nothing. + const std::string directory = ExecutableDirectory(); + + if (directory.empty() || !SetCurrentDirectoryA(directory.c_str())) + { + g_status.dwWin32ExitCode = ERROR_PATH_NOT_FOUND; + PublishStatus(SERVICE_STOPPED); + return; + } + + g_inBackground.store(true, std::memory_order_release); + + PublishStatus(SERVICE_RUNNING); + + // The server's own loop, handed over by the caller. + g_exitCode = g_serve ? g_serve() : EXIT_FAILURE; + + PublishStatus(SERVICE_STOP_PENDING); + + g_status.dwControlsAccepted = 0; + g_status.dwWin32ExitCode = + g_exitCode == EXIT_SUCCESS ? NO_ERROR : ERROR_SERVICE_SPECIFIC_ERROR; + g_status.dwServiceSpecificExitCode = DWORD(g_exitCode); + PublishStatus(SERVICE_STOPPED); +} + +} // namespace + +bool HasServiceManager() +{ + return true; +} + +bool Install(const Options& options) +{ + SC_HANDLE manager = OpenSCManagerA(nullptr, nullptr, SC_MANAGER_CREATE_SERVICE); + + if (!manager) + { + sLog.outError("SERVICE: no access to the service control manager." + " Run as administrator."); + return false; + } + + const std::string binary = ExecutablePath(); + + if (binary.empty()) + { + CloseServiceHandle(manager); + sLog.outError("SERVICE: cannot determine this executable's path."); + return false; + } + + // Built as a string, not appended into a fixed array: the path alone can + // fill MAX_PATH, and the arguments still have to go somewhere. Quoted, so + // that a Program Files install is not read as a binary plus two arguments. + const std::string command = "\"" + binary + "\" -s run"; + + SC_HANDLE service = CreateServiceA( + manager, + options.serviceName.c_str(), + options.serviceDisplayName.c_str(), + SERVICE_ALL_ACCESS, + // Not SERVICE_INTERACTIVE_PROCESS: it has been ignored since Vista and + // asking for it only means asking to run in session 0 with a desktop. + SERVICE_WIN32_OWN_PROCESS, + SERVICE_AUTO_START, + // Not SERVICE_ERROR_IGNORE, which is what a failed start being invisible + // in the event log looks like from the outside. + SERVICE_ERROR_NORMAL, + command.c_str(), + nullptr, nullptr, nullptr, nullptr, nullptr); + + if (!service) + { + const unsigned error = unsigned(GetLastError()); + CloseServiceHandle(manager); + sLog.outError("SERVICE: cannot register '%s': error %u", + options.serviceName.c_str(), error); + return false; + } + + // Linked directly rather than looked up through GetProcAddress: every + // Windows this server runs on exports it. + SERVICE_DESCRIPTIONA description{}; + description.lpDescription = const_cast(options.serviceDescription.c_str()); + ChangeServiceConfig2A(service, SERVICE_CONFIG_DESCRIPTION, &description); + + SC_ACTION restart{}; + restart.Type = SC_ACTION_RESTART; + restart.Delay = 10000; + + SERVICE_FAILURE_ACTIONSA failure{}; + failure.dwResetPeriod = INFINITE; + failure.cActions = 1; + failure.lpsaActions = &restart; + ChangeServiceConfig2A(service, SERVICE_CONFIG_FAILURE_ACTIONS, &failure); + + CloseServiceHandle(service); + CloseServiceHandle(manager); + + sLog.outString("SERVICE: '%s' installed.", options.serviceName.c_str()); + return true; +} + +bool Uninstall(const Options& options) +{ + SC_HANDLE manager = OpenSCManagerA(nullptr, nullptr, SC_MANAGER_CONNECT); + + if (!manager) + { + sLog.outError("SERVICE: no access to the service control manager." + " Run as administrator."); + return false; + } + + SC_HANDLE service = OpenServiceA(manager, options.serviceName.c_str(), + SERVICE_QUERY_STATUS | DELETE); + + if (!service) + { + CloseServiceHandle(manager); + sLog.outError("SERVICE: '%s' is not installed.", options.serviceName.c_str()); + return false; + } + + SERVICE_STATUS status{}; + bool removed = false; + + if (QueryServiceStatus(service, &status) && status.dwCurrentState == SERVICE_STOPPED) + { + removed = DeleteService(service) != FALSE; + + if (!removed) + { + sLog.outError("SERVICE: cannot remove '%s': error %u", + options.serviceName.c_str(), unsigned(GetLastError())); + } + } + else + { + // Reported as the failure it is: deleting nothing and returning success + // is how "uninstall" comes to leave the service installed. + sLog.outError("SERVICE: '%s' is still running; stop it before removing it.", + options.serviceName.c_str()); + } + + CloseServiceHandle(service); + CloseServiceHandle(manager); + + if (removed) + { + sLog.outString("SERVICE: '%s' removed.", options.serviceName.c_str()); + } + + return removed; +} + +int RunInBackground(const Options& options, const std::function& serve) +{ + g_serve = serve; + g_serviceName = options.serviceName; + + // A non-const copy because the table's name field is char*, and the API does + // not promise not to touch it. + std::vector name(g_serviceName.begin(), g_serviceName.end()); + name.push_back('\0'); + + SERVICE_TABLE_ENTRYA table[] = + { + { name.data(), ServiceMain }, + { nullptr, nullptr } + }; + + if (!StartServiceCtrlDispatcherA(table)) + { + sLog.outError("SERVICE: cannot start the control dispatcher: error %u", + unsigned(GetLastError())); + return EXIT_FAILURE; + } + + return g_exitCode; +} + +void ReportReady() +{ + // The service manager was told the service was running before the loop was + // entered, which is as much as it wants to know. +} + +bool Stop(const Options& options) +{ + SC_HANDLE manager = OpenSCManagerA(nullptr, nullptr, SC_MANAGER_CONNECT); + + if (!manager) + { + sLog.outError("SERVICE: no access to the service control manager."); + return false; + } + + SC_HANDLE service = OpenServiceA(manager, options.serviceName.c_str(), SERVICE_STOP); + bool stopped = false; + + if (service) + { + SERVICE_STATUS status{}; + stopped = ControlService(service, SERVICE_CONTROL_STOP, &status) != FALSE; + + if (!stopped) + { + sLog.outError("SERVICE: cannot stop '%s': error %u", + options.serviceName.c_str(), unsigned(GetLastError())); + } + + CloseServiceHandle(service); + } + else + { + sLog.outError("SERVICE: '%s' is not installed.", options.serviceName.c_str()); + } + + CloseServiceHandle(manager); + return stopped; +} + +bool IsRunningInBackground() +{ + return g_inBackground.load(std::memory_order_acquire); +} + +bool StopRequested() +{ + return g_stopRequested.load(std::memory_order_acquire); +} + +bool IsPaused() +{ + return g_paused.load(std::memory_order_acquire); +} + +} // namespace Process From 3acfc9b721ac9b0402d71ff9a5f81757cd508f63 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 01:59:02 +0300 Subject: [PATCH 2/9] Replace EventProcessor with the tested implementation Implemented: unique_ptr ownership throughout; AddEvent reports refusal. Implemented: Reschedule(), for an event re-adding itself from inside Execute. Implemented: RequestAbort/IsAbortRequested/AddedAt/ScheduledFor; to_Abort, m_addTime and m_execTime are no longer public. Implemented: 14 hostile cases in src/tests/EventProcessorHostileTest.cpp. Fixed: m_aborting was set but never read, so an event queued from an Abort handler landed in a container cleared two lines later, and leaked. Fixed: Abort was delivered twice on the non-forced path -- an undeletable event was aborted, left queued, then aborted again by the next Update. For SpellEvent that meant Spell::cancel() twice on one spell. Fixed: the non-forced path iterated m_events in place while calling Abort, which is virtual; a re-entrant KillAllEvents(true) invalidated the iterator. Fixed: an event re-added for the tick being processed was picked up again in the same loop, and Update never returned. Fixed: a forced KillAllEvents raised from inside an Abort was silently downgraded to non-forced, because the outer pass had already moved the queue aside. Fixed: Reschedule of an already-queued event produced two owners and a double free. Co-Authored-By: Claude Opus 5 --- src/game/BattleGround/BattleGroundQueue.cpp | 10 +- src/game/Object/Creature.cpp | 5 +- src/game/Object/CreatureAI.cpp | 5 +- src/game/Object/Unit.cpp | 3 +- src/game/WorldHandlers/Spell.cpp | 20 +- src/shared/Utilities/EventProcessor.cpp | 297 +++++++++---- src/shared/Utilities/EventProcessor.h | 212 +++++++--- src/tests/CMakeLists.txt | 1 + src/tests/EventProcessorHostileTest.cpp | 440 ++++++++++++++++++++ 9 files changed, 831 insertions(+), 162 deletions(-) create mode 100644 src/tests/EventProcessorHostileTest.cpp diff --git a/src/game/BattleGround/BattleGroundQueue.cpp b/src/game/BattleGround/BattleGroundQueue.cpp index d8bdd678c..965b5d916 100644 --- a/src/game/BattleGround/BattleGroundQueue.cpp +++ b/src/game/BattleGround/BattleGroundQueue.cpp @@ -511,11 +511,13 @@ bool BattleGroundQueue::InviteGroupToBG(GroupQueueInfo* ginfo, BattleGround* bg, plr->SetInviteForBattleGroundQueueType(bgQueueTypeId, ginfo->IsInvitedToBGInstanceGUID); // create remind invite events - BGQueueInviteEvent* inviteEvent = new BGQueueInviteEvent(plr->GetObjectGuid(), ginfo->IsInvitedToBGInstanceGUID, bgTypeId, ginfo->RemoveInviteTime); - plr->m_Events.AddEvent(inviteEvent, plr->m_Events.CalculateTime(INVITATION_REMIND_TIME)); + plr->m_Events.AddEvent( + std::unique_ptr(new BGQueueInviteEvent(plr->GetObjectGuid(), ginfo->IsInvitedToBGInstanceGUID, bgTypeId, ginfo->RemoveInviteTime)), + plr->m_Events.CalculateTime(INVITATION_REMIND_TIME)); // create automatic remove events - BGQueueRemoveEvent* removeEvent = new BGQueueRemoveEvent(plr->GetObjectGuid(), ginfo->IsInvitedToBGInstanceGUID, bgTypeId, bgQueueTypeId, ginfo->RemoveInviteTime); - plr->m_Events.AddEvent(removeEvent, plr->m_Events.CalculateTime(INVITE_ACCEPT_WAIT_TIME)); + plr->m_Events.AddEvent( + std::unique_ptr(new BGQueueRemoveEvent(plr->GetObjectGuid(), ginfo->IsInvitedToBGInstanceGUID, bgTypeId, bgQueueTypeId, ginfo->RemoveInviteTime)), + plr->m_Events.CalculateTime(INVITE_ACCEPT_WAIT_TIME)); WorldPacket data; diff --git a/src/game/Object/Creature.cpp b/src/game/Object/Creature.cpp index 50a954e95..8ddad35ca 100644 --- a/src/game/Object/Creature.cpp +++ b/src/game/Object/Creature.cpp @@ -2157,9 +2157,8 @@ void Creature::ForcedDespawn(uint32 timeMSToDespawn) { if (timeMSToDespawn) { - ForcedDespawnDelayEvent* pEvent = new ForcedDespawnDelayEvent(*this); - - m_Events.AddEvent(pEvent, m_Events.CalculateTime(timeMSToDespawn)); + m_Events.AddEvent(std::unique_ptr(new ForcedDespawnDelayEvent(*this)), + m_Events.CalculateTime(timeMSToDespawn)); return; } diff --git a/src/game/Object/CreatureAI.cpp b/src/game/Object/CreatureAI.cpp index c241a1aff..74d7bed22 100644 --- a/src/game/Object/CreatureAI.cpp +++ b/src/game/Object/CreatureAI.cpp @@ -653,8 +653,9 @@ void CreatureAI::SendAIEventAround(AIEventType eventType, Unit* pInvoker, uint32 if (!receiverList.empty()) { - AiDelayEventAround* e = new AiDelayEventAround(eventType, pInvoker ? pInvoker->GetObjectGuid() : ObjectGuid(), *m_creature, receiverList, miscValue); - m_creature->m_Events.AddEvent(e, m_creature->m_Events.CalculateTime(uiDelay)); + m_creature->m_Events.AddEvent( + std::unique_ptr(new AiDelayEventAround(eventType, pInvoker ? pInvoker->GetObjectGuid() : ObjectGuid(), *m_creature, receiverList, miscValue)), + m_creature->m_Events.CalculateTime(uiDelay)); } } } diff --git a/src/game/Object/Unit.cpp b/src/game/Object/Unit.cpp index f559b9575..ad24426f1 100644 --- a/src/game/Object/Unit.cpp +++ b/src/game/Object/Unit.cpp @@ -6455,7 +6455,8 @@ void Unit::ScheduleAINotify(uint32 delay) { if (!IsAINotifyScheduled()) { - m_Events.AddEvent(new RelocationNotifyEvent(*this), m_Events.CalculateTime(delay)); + m_Events.AddEvent(std::unique_ptr(new RelocationNotifyEvent(*this)), + m_Events.CalculateTime(delay)); } } diff --git a/src/game/WorldHandlers/Spell.cpp b/src/game/WorldHandlers/Spell.cpp index 713379f50..de7509b98 100644 --- a/src/game/WorldHandlers/Spell.cpp +++ b/src/game/WorldHandlers/Spell.cpp @@ -599,8 +599,8 @@ SpellCastResult Spell::prepare(SpellCastTargets const* targets, Aura* triggeredB } // create and add update event for this spell - SpellEvent* Event = new SpellEvent(this); - m_caster->m_Events.AddEvent(Event, m_caster->m_Events.CalculateTime(1)); + m_caster->m_Events.AddEvent(std::unique_ptr(new SpellEvent(this)), + m_caster->m_Events.CalculateTime(1)); // Prevent casting at cast another spell (ServerSide check) if (!m_IsTriggeredSpell && m_caster->IsNonMeleeSpellCasted(false, true, true)) @@ -1039,9 +1039,12 @@ bool SpellEvent::Execute(uint64 e_time, uint32 p_time) uint64 n_offset = m_Spell->handle_delayed(t_offset); if (n_offset) { - // re-add us to the queue - m_Spell->GetCaster()->m_Events.AddEvent(this, m_Spell->GetDelayStart() + n_offset, false); - return false; // event not complete + // re-add us to the queue; false means "not complete, the + // queue owns me again". A refused re-add means the + // caster's processor is tearing down and nobody adopted + // us, so we ask to be destroyed instead of leaking. + return !m_Spell->GetCaster()->m_Events.Reschedule( + this, m_Spell->GetDelayStart() + n_offset); } // event complete // finish update event will be re-added automatically at the end of routine) @@ -1052,8 +1055,8 @@ bool SpellEvent::Execute(uint64 e_time, uint32 p_time) // delaying had just started, record the moment m_Spell->SetDelayStart(e_time); // re-plan the event for the delay moment - m_Spell->GetCaster()->m_Events.AddEvent(this, e_time + m_Spell->GetDelayMoment(), false); - return false; // event not complete + return !m_Spell->GetCaster()->m_Events.Reschedule( + this, e_time + m_Spell->GetDelayMoment()); } break; } @@ -1066,8 +1069,7 @@ bool SpellEvent::Execute(uint64 e_time, uint32 p_time) } // spell processing not complete, plan event on the next update interval - m_Spell->GetCaster()->m_Events.AddEvent(this, e_time + 1, false); - return false; // event not complete + return !m_Spell->GetCaster()->m_Events.Reschedule(this, e_time + 1); } /** diff --git a/src/shared/Utilities/EventProcessor.cpp b/src/shared/Utilities/EventProcessor.cpp index 4d3291ce7..cb4707956 100644 --- a/src/shared/Utilities/EventProcessor.cpp +++ b/src/shared/Utilities/EventProcessor.cpp @@ -23,123 +23,268 @@ * and lore are copyrighted by Blizzard Entertainment, Inc. */ -#include #include "EventProcessor.h" -/** - * @brief Construct a new Event Processor::Event Processor object - * Initializes member variables m_time and m_aborting. - */ -EventProcessor::EventProcessor() +#include + +namespace Events { - m_time = 0; - m_aborting = false; -} -/** - * @brief Destroy the Event Processor::Event Processor object - * Calls KillAllEvents with force set to true. - */ EventProcessor::~EventProcessor() { KillAllEvents(true); } -/** - * @brief Updates the event processor with the given time. - * - * @param p_time Time to update the event processor with. - */ -void EventProcessor::Update(uint32 p_time) +bool EventProcessor::DeliverAbort(BasicEvent& event, std::uint64_t time) { - // update time - m_time += p_time; + if (event.m_aborted) + { + return false; + } + + event.m_aborted = true; + event.Abort(time); + return true; +} + +void EventProcessor::Update(std::uint32_t elapsed) +{ + m_time += elapsed; + ++m_pass; - // main event loop - EventList::iterator i; - while (((i = m_events.begin()) != m_events.end()) && i->first <= m_time) + while (!m_events.empty()) { - // get and remove event from queue - BasicEvent* Event = i->second; - m_events.erase(i); + auto it = m_events.begin(); + if (it->first > m_time) + { + break; + } - if (!Event->to_Abort) + // Queued during THIS pass, so it waits for the next one. An event that + // re-adds itself for the current moment would otherwise be picked up + // again immediately, and Update would never return. + if (it->second && it->second->m_queuedPass == m_pass) { - if (Event->Execute(m_time, p_time)) - { - // completely destroy event if it is not re-added - delete Event; - } + break; } - else + + // Ownership moves OUT of the map before the event runs. That ordering is + // what makes re-entrancy safe: while Execute is running, the map holds + // no pointer to this event, so anything Execute triggers -- another + // Update, a KillAllEvents, the owner's destruction -- cannot reach it + // and cannot destroy it twice. + std::unique_ptr event = std::move(it->second); + m_events.erase(it); + + if (!event) + { + continue; + } + + if (event->IsAbortRequested()) { - Event->Abort(m_time); - delete Event; + DeliverAbort(*event, m_time); + continue; // the unique_ptr destroys it + } + + if (!event->Execute(m_time, elapsed)) + { + // The event re-added itself, here or elsewhere, and that insertion + // is now its owner. Releasing is what stops this unique_ptr from + // destroying an object the queue is holding. + // + // An Execute that returns false WITHOUT re-adding leaks the event. + // That is the contract's one sharp edge and it cannot be detected + // from here -- the event may legitimately have gone to a processor + // this one has never heard of. + event.release(); } } } -/** - * @brief Kills all events in the event processor. - * - * @param force If true, forces the deletion of all events. - */ void EventProcessor::KillAllEvents(bool force) { - // prevent event insertions + // Read by AddEvent and Reschedule, which refuse while it is set: an event + // queued from an Abort handler would land in a container that is being + // emptied two lines later. + // + // ===== SAVED AND RESTORED, BECAUSE THIS NESTS ===== + // + // Abort() is virtual and runs game code, and game code can call + // KillAllEvents again. An inner call that finished by storing `false` + // outright would leave the OUTER call delivering aborts with the guard + // switched off, and an AddEvent from any later handler accepted into a + // processor that is in the middle of destroying its queue. + // + // Restoring the previous value instead makes the depth irrelevant: the inner + // call puts back `true`, and only the outermost one puts back `false`. + // ================================================== + const bool wasAborting = m_aborting; m_aborting = true; - // first, abort all existing events - for (EventList::iterator i = m_events.begin(); i != m_events.end();) + if (force) { - EventList::iterator i_old = i; - ++i; + // Tell any non-forced pass further up the stack that its survivors are + // not to be re-queued. Its container is out of reach from here. + m_forceRequested = true; - i_old->second->to_Abort = true; - i_old->second->Abort(m_time); - if (force || i_old->second->IsDeletable()) - { - delete i_old->second; + // Move the whole queue out FIRST, then destroy. + // + // Destroying them in place would leave the map full of dangling + // pointers between the first destruction and the clear -- and Abort() is + // virtual and runs game code, so anything it touches that reaches back + // into this processor would walk them. + std::multimap> doomed; + doomed.swap(m_events); - if (!force) // need per-element cleanup + for (auto& entry : doomed) + { + if (entry.second) { - m_events.erase(i_old); + entry.second->RequestAbort(); + DeliverAbort(*entry.second, m_time); } } + + // doomed goes out of scope here and destroys every event. + // + // The request is left standing for a non-forced pass higher up to read, + // and cleared only by the outermost call -- otherwise it would survive + // into the next unrelated kill and drop survivors nobody asked to drop. + if (!wasAborting) + { + m_forceRequested = false; + } + + m_aborting = wasAborting; + return; } - // fast clear event list (in force case) - if (force) + // ===== NOT ITERATED WHILE GAME CODE RUNS ===== + // + // Non-forced: abort everything, but keep what is not yet deletable. Abort is + // virtual, it runs game code, and that code can call KillAllEvents(true) -- + // which swaps the whole map out from under any iterator this function is + // holding. AddEvent is refused during an abort, so insertion is not the + // hazard; the swap is, and no flag prevents it. + // + // So the queue is moved out first and put back afterwards, the same shape the + // forced branch already uses. While the handlers run, m_events is a container + // they are welcome to do anything to. + // ============================================= + std::multimap> pending; + pending.swap(m_events); + + std::multimap> survivors; + + // Saved and restored for the same reason m_aborting is: this nests. + const bool hadForceRequest = m_forceRequested; + m_forceRequested = false; + + for (auto& entry : pending) { - m_events.clear(); + if (!entry.second) + { + continue; + } + + entry.second->RequestAbort(); + DeliverAbort(*entry.second, m_time); + + if (!entry.second->IsDeletable()) + { + // Stays queued. A later Update will find the abort request and + // destroy it -- WITHOUT calling Abort again, because m_aborted is + // now set. + survivors.emplace(entry.first, std::move(entry.second)); + } + + // Anything else is destroyed with `pending`, at the end of this scope. } + + // A handler asked for a forced kill while this pass was running, and it + // found nothing to destroy because the queue was already in `pending`. + // Honouring it here is what stops "force" from being downgraded: the + // survivors are dropped rather than re-queued, and `survivors` destroys + // them on the way out. They were aborted once already, and DeliverAbort + // will not fire a second time. + if (!m_forceRequested) + { + // Whatever a handler queued in the meantime keeps its place; the + // survivors are merged back in rather than overwriting it. + for (auto& entry : survivors) + { + m_events.emplace(entry.first, std::move(entry.second)); + } + } + + m_forceRequested = hadForceRequest; + m_aborting = wasAborting; } -/** - * @brief Adds an event to the event processor. - * - * @param Event Pointer to the event to add. - * @param e_time Execution time of the event. - * @param set_addtime If true, sets the add time of the event. - */ -void EventProcessor::AddEvent(BasicEvent* Event, uint64 e_time, bool set_addtime) +bool EventProcessor::AddEvent(std::unique_ptr event, + std::uint64_t executionTime, + bool setAddTime) { - if (set_addtime) + if (!event) { - Event->m_addTime = m_time; + return false; } - Event->m_execTime = e_time; - m_events.insert(std::pair(e_time, Event)); + if (m_aborting) + { + // Refused, and cleaned up rather than dropped. The event is aborted so + // that whatever it was holding is released on the way out. + event->RequestAbort(); + DeliverAbort(*event, m_time); + return false; + } + + if (setAddTime) + { + event->m_addTime = m_time; + } + event->m_execTime = executionTime; + event->m_queuedPass = m_pass; + + m_events.emplace(executionTime, std::move(event)); + return true; } -/** - * @brief Calculates the time with the given offset. - * - * @param t_offset Time offset to add. - * @return uint64 Calculated time. - */ -uint64 EventProcessor::CalculateTime(uint64 t_offset) const +bool EventProcessor::Reschedule(BasicEvent* event, + std::uint64_t executionTime, + bool setAddTime) { - return m_time + t_offset; + if (!event) + { + return false; + } + + if (m_aborting) + { + return false; + } + + // Already queued: adopting it a second time would put two unique_ptrs on one + // object, and the second destruction is a double free. Linear, but the scan + // only runs on the re-add path and these queues are short. + for (const auto& entry : m_events) + { + if (entry.second.get() == event) + { + return false; + } + } + + if (setAddTime) + { + event->m_addTime = m_time; + } + event->m_execTime = executionTime; + event->m_queuedPass = m_pass; + + m_events.emplace(executionTime, std::unique_ptr(event)); + return true; } + +} // namespace Events diff --git a/src/shared/Utilities/EventProcessor.h b/src/shared/Utilities/EventProcessor.h index 300540087..39142f02a 100644 --- a/src/shared/Utilities/EventProcessor.h +++ b/src/shared/Utilities/EventProcessor.h @@ -26,123 +26,201 @@ #ifndef MANGOS_H_EVENTPROCESSOR #define MANGOS_H_EVENTPROCESSOR -#include "Platform/Define.h" +#include #include +#include + +namespace Events +{ + +class EventProcessor; /** - * @brief Note. All times are in milliseconds here. + * @brief Something to happen later. All times are milliseconds. + * + * Subclass it, override Execute, hand it to an EventProcessor. The processor + * owns it from that moment until one of two things happens: + * + * Execute returns TRUE the event is finished and the processor destroys it. + * + * Execute returns FALSE the event has RE-INSERTED ITSELF, into this + * processor or another one, and the processor gives up + * ownership without destroying it. * + * The second is not a hypothetical: a spell in flight re-adds itself on every + * update until it lands, sometimes into a different caster's processor. It is + * also the sharp edge -- an Execute that returns false without re-adding leaks + * the event, silently and forever, and nothing can detect it from here. */ class BasicEvent { public: + + BasicEvent() = default; + virtual ~BasicEvent() = default; + + // Events are owned through pointers and identified by address; copying + // one would produce a second object the processor knows nothing about. + BasicEvent(const BasicEvent&) = delete; + BasicEvent& operator=(const BasicEvent&) = delete; + /** - * @brief Construct a new Basic Event object - * Initializes member variables to_Abort, m_addTime, and m_execTime. + * @param executionTime The processor's clock at the moment of execution. + * @param elapsed Milliseconds since the previous update. + * @return true to be destroyed, false if the event has re-added itself. */ - BasicEvent() - : to_Abort(false), m_addTime(0), m_execTime(0) // Initialize member variables + virtual bool Execute(std::uint64_t /*executionTime*/, std::uint32_t /*elapsed*/) { + return true; } - /** - * @brief Destroy the Basic Event object - * Override destructor to perform some actions on event removal. - */ - virtual ~BasicEvent() - { - }; + /// False to survive a non-forced KillAllEvents. + virtual bool IsDeletable() const { return true; } - /** - * @brief This method executes when the event is triggered - * - * @param e_time Execution time - * @param p_time Update interval - * @return bool Return false if event does not want to be deleted - */ - virtual bool Execute(uint64 /*e_time*/, uint32 /*p_time*/) { return true; } + /// Called instead of Execute when the event is cancelled. Exactly once. + virtual void Abort(std::uint64_t /*executionTime*/) {} /** - * @brief This event can be safely deleted + * @brief Ask for this event to be cancelled rather than executed. * - * @return bool + * A request from outside, so it is writable from outside -- but paired + * with a separate flag the processor owns, so "someone asked" and "the + * abort has been delivered" stay distinct. */ - virtual bool IsDeletable() const { return true; } + void RequestAbort() { m_abortRequested = true; } + bool IsAbortRequested() const { return m_abortRequested; } + + /// When the event was queued, and when it is due. + std::uint64_t AddedAt() const { return m_addTime; } + std::uint64_t ScheduledFor() const { return m_execTime; } + + private: + + friend class EventProcessor; + + bool m_abortRequested = false; /** - * @brief This method executes when the event is aborted + * @brief Whether Abort() has already been delivered. * - * @param e_time Execution time + * A non-forced KillAllEvents aborts every event but leaves the ones that + * are not yet deletable in the queue; a later Update finds them and + * destroys them. Without this flag it would abort them a SECOND time -- + * and an Abort that releases a resource, refunds a cost or notifies a + * player would do it twice, with nothing in the event's own code to + * suggest that could happen. */ - virtual void Abort(uint64 /*e_time*/) {} + bool m_aborted = false; - bool to_Abort; /**< Set by externals when the event is aborted, aborted events don't execute and get Abort call when deleted */ + std::uint64_t m_addTime = 0; + std::uint64_t m_execTime = 0; - // These can be used for time offset control - uint64 m_addTime; /**< Time when the event was added to queue, filled by event handler */ - uint64 m_execTime; /**< Planned time of next execution, filled by event handler */ + /// Which Update pass queued this, so a same-tick re-add waits for the + /// next one instead of spinning inside the current loop. + std::uint64_t m_queuedPass = 0; }; /** - * @brief Typedef for a multimap of events - * - */ -typedef std::multimap EventList; - -/** - * @brief Event Processor class + * @brief A time-ordered queue of events, driven by the owner's update tick. * + * One of these lives on every Unit and every Player. It is not thread-safe and + * does not need to be: it is driven from the thread that updates its owner. */ class EventProcessor { public: - /** - * @brief Construct a new Event Processor object - * Initializes member variables m_time and m_aborting. - */ - EventProcessor(); - /** - * @brief Destroy the Event Processor object - * Calls KillAllEvents with force set to true. - */ + EventProcessor() = default; ~EventProcessor(); + EventProcessor(const EventProcessor&) = delete; + EventProcessor& operator=(const EventProcessor&) = delete; + + /// Advance the clock and run everything now due. + void Update(std::uint32_t elapsed); + /** - * @brief Updates the event processor with the given time + * @brief Cancel everything. * - * @param p_time Time to update the event processor with + * @param force true destroys every event regardless of IsDeletable(). + * false leaves undeletable events queued -- they will be + * destroyed by a later Update, without a second Abort. */ - void Update(uint32 p_time); + void KillAllEvents(bool force); /** - * @brief Kills all events in the event processor + * @brief Queue an event, taking ownership. * - * @param force If true, forces the deletion of all events + * @return false if the processor is tearing down, in which case the + * event is aborted and destroyed rather than queued. + * + * An Abort handler is exactly where a dying object queues its cleanup, + * so insertion during teardown is a real path and must be refused -- + * anything accepted then would be dropped by the teardown that is + * already in progress. The refusal is reported rather than swallowed, so + * a caller that queued something important finds out. */ - void KillAllEvents(bool force); + bool AddEvent(std::unique_ptr event, + std::uint64_t executionTime, + bool setAddTime = true); /** - * @brief Adds an event to the event processor + * @brief Re-queue an event that is executing right now. + * + * ONLY legal from inside that event's own Execute, which must then + * return false. That pair is the contract: Execute says "I did not + * finish", and this says where the still-living event went. * - * @param Event Pointer to the event to add - * @param e_time Execution time of the event - * @param set_addtime If true, sets the add time of the event + * Takes a raw pointer BECAUSE the event is mid-Execute and cannot hand + * over a unique_ptr to itself. The processor running it releases + * ownership precisely when Execute returns false, so exactly one owner + * exists at every moment. + * + * @return false if the target processor is tearing down, or if the event + * is already queued; the event is then NOT adopted and the + * caller still owns it -- which means Execute must return true. */ - void AddEvent(BasicEvent* Event, uint64 e_time, bool set_addtime = true); + bool Reschedule(BasicEvent* event, + std::uint64_t executionTime, + bool setAddTime = false); + + /// The processor's clock plus an offset -- how callers name a due time. + std::uint64_t CalculateTime(std::uint64_t offset) const { return m_time + offset; } + + std::uint64_t Now() const { return m_time; } + + bool IsEmpty() const { return m_events.empty(); } + + private: + + /// Deliver Abort exactly once. Returns false if it had already been sent. + static bool DeliverAbort(BasicEvent& event, std::uint64_t time); + + std::multimap> m_events; + + std::uint64_t m_time = 0; + std::uint64_t m_pass = 0; + bool m_aborting = false; /** - * @brief Calculates the time with the given offset + * @brief A forced kill was asked for while a non-forced one was running. * - * @param t_offset Time offset to add - * @return uint64 Calculated time + * The non-forced pass moves the queue into a local before running any + * Abort handler, so a forced kill reaching the processor from inside one + * finds m_events already empty and destroys nothing. Left at that, the + * outer pass would then re-queue its undeletable survivors and the + * caller's "force" would have been silently downgraded. This flag is how + * the outer pass learns it must drop them instead. */ - uint64 CalculateTime(uint64 t_offset) const; - - protected: - uint64 m_time; /**< Current time in milliseconds */ - EventList m_events; /**< List of events */ - bool m_aborting; /**< Flag indicating if the event processor is aborting */ + bool m_forceRequested = false; }; +} // namespace Events + +// The server declares its events and its processors unqualified, in game headers +// that have nothing else to say about namespaces. Hoisting the two names keeps +// the structure aligned with the shared tree without renaming every call site. +using Events::BasicEvent; +using Events::EventProcessor; + #endif diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index d9252f31e..756f5484b 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -39,6 +39,7 @@ set(SRC_GRP_TESTS Utf8Test.cpp ConfigTest.cpp ByteBufferTest.cpp + EventProcessorHostileTest.cpp SessionMailboxTest.cpp SessionProtocolPolicyTest.cpp WorldGatewayAccountTest.cpp diff --git a/src/tests/EventProcessorHostileTest.cpp b/src/tests/EventProcessorHostileTest.cpp new file mode 100644 index 000000000..03b9d22f0 --- /dev/null +++ b/src/tests/EventProcessorHostileTest.cpp @@ -0,0 +1,440 @@ +/** + * SPDX-License-Identifier: GPL-3.0-or-later + * + * MaNGOS is a full featured server for World of Warcraft, supporting + * the following clients: 1.12.x, 2.4.3, 3.3.5a, 4.3.4a and 5.4.8 + * + * Copyright (C) 2005-2026 MaNGOS + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * World of Warcraft, and all World of Warcraft or Warcraft art, images, + * and lore are copyrighted by Blizzard Entertainment, Inc. + */ + +/** + * @file EventProcessorHostileTest.cpp + * @brief EventProcessor driven by events that fight back. + * + * Every event handler here is game code, and game code is allowed to do the + * worst thing available to it at that moment: queue another event from inside an + * Abort, destroy the whole queue from inside an Abort, re-add itself twice from + * inside its own Execute, or re-add itself for the very instant that is being + * processed. Each of those reaches the processor while it is mid-operation on + * its own container, and each has a wrong answer that looks like nothing at all + * until a player logs out during a spell cast. + * + * The happy path gets one case at the end. Everything above it is the point. + */ + +#include "TestHarness.h" + +#include "Utilities/EventProcessor.h" + +#include +#include +#include + +namespace +{ + /// Fires up to a cap, then reports itself finished. + struct CountingEvent : BasicEvent + { + CountingEvent(int& fires, int cap) + : m_fires(fires) + , m_cap(cap) + { + } + + bool Execute(std::uint64_t, std::uint32_t) override + { + ++m_fires; + return m_fires >= m_cap; + } + + int& m_fires; + int m_cap; + }; + + /// Counts how many times Abort reaches it. Must never exceed one. + struct AbortCountingEvent : BasicEvent + { + AbortCountingEvent(int& aborts, bool deletable) + : m_aborts(aborts) + , m_deletable(deletable) + { + } + + void Abort(std::uint64_t) override { ++m_aborts; } + bool IsDeletable() const override { return m_deletable; } + + int& m_aborts; + bool m_deletable; + }; + + /// Tries to adopt itself twice from one Execute. + struct DoubleRescheduleEvent : BasicEvent + { + DoubleRescheduleEvent(EventProcessor& processor, bool& secondAccepted) + : m_processor(processor) + , m_secondAccepted(secondAccepted) + { + } + + bool Execute(std::uint64_t, std::uint32_t) override + { + const bool first = m_processor.Reschedule(this, m_processor.CalculateTime(10)); + const bool second = m_processor.Reschedule(this, m_processor.CalculateTime(20)); + m_secondAccepted = second; + + // If the second adoption succeeded, two unique_ptrs alias this + // object and returning false would leave both of them live. The + // contract is that the second is refused; returning "still owned by + // the queue" is only safe because of that. + return !first; + } + + EventProcessor& m_processor; + bool& m_secondAccepted; + }; + + /// Re-adds itself for the instant currently being processed. + struct SameTickEvent : BasicEvent + { + SameTickEvent(EventProcessor& processor, int& fires) + : m_processor(processor) + , m_fires(fires) + { + } + + bool Execute(std::uint64_t, std::uint32_t) override + { + ++m_fires; + if (m_fires >= 32) + { + return true; + } + + return !m_processor.Reschedule(this, m_processor.Now()); + } + + EventProcessor& m_processor; + int& m_fires; + }; + + /// Queues a new event from inside its own Abort. + struct AbortAddsEvent : BasicEvent + { + AbortAddsEvent(EventProcessor& processor, int& accepted, int& fires) + : m_processor(processor) + , m_accepted(accepted) + , m_fires(fires) + { + } + + void Abort(std::uint64_t) override + { + if (m_processor.AddEvent( + std::unique_ptr(new CountingEvent(m_fires, 1)), + m_processor.CalculateTime(1))) + { + ++m_accepted; + } + } + + EventProcessor& m_processor; + int& m_accepted; + int& m_fires; + }; + + /// Destroys the whole queue from inside its own Abort. + struct AbortKillsEvent : BasicEvent + { + AbortKillsEvent(EventProcessor& processor, bool force) + : m_processor(processor) + , m_force(force) + { + } + + void Abort(std::uint64_t) override { m_processor.KillAllEvents(m_force); } + + EventProcessor& m_processor; + bool m_force; + }; +} + +// ===== 1. Unhappy path: refusals ===== + +TEST(EventProcessor_NullEventIsRefused) +{ + EventProcessor processor; + + CHECK(!processor.AddEvent(nullptr, 1)); + CHECK(processor.IsEmpty()); + + CHECK(!processor.Reschedule(nullptr, 1)); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_EmptyUpdateStillAdvancesTheClock) +{ + EventProcessor processor; + + processor.Update(50); + + CHECK(processor.Now() == 50); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_AddDuringForcedKillIsRefused) +{ + // An Abort handler is where a dying object queues its cleanup. Accepting it + // would drop the event on the floor: the teardown that triggered the Abort + // has already decided what it is destroying. + EventProcessor processor; + int accepted = 0; + int fires = 0; + + processor.AddEvent( + std::unique_ptr(new AbortAddsEvent(processor, accepted, fires)), 100); + processor.KillAllEvents(true); + + CHECK(accepted == 0); + CHECK(fires == 0); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_RefusedAddStillAbortsTheEvent) +{ + // Refusing must not mean leaking: whatever the event was holding has to be + // released, so the refusal path delivers Abort on the way out. + EventProcessor processor; + int aborts = 0; + + processor.AddEvent(std::unique_ptr(new AbortKillsEvent(processor, true)), 10); + + // Drive a forced kill; the handler above re-enters KillAllEvents, and while + // it runs the guard is set, so this insertion is refused. + int accepted = 0; + int fires = 0; + processor.AddEvent( + std::unique_ptr(new AbortAddsEvent(processor, accepted, fires)), 20); + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, true)), 30); + + processor.KillAllEvents(true); + + CHECK(accepted == 0); + CHECK(aborts == 1); + CHECK(processor.IsEmpty()); +} + +// ===== 2. Hostile: handlers that re-enter the processor ===== + +TEST(EventProcessor_AbortIsDeliveredExactlyOnceAcrossANonForcedKill) +{ + // ===== THE DOUBLE ABORT ===== + // + // A non-forced kill aborts everything but leaves undeletable events queued; + // a later Update finds them and destroys them. Without a flag recording that + // the abort was already sent, that second visit calls Abort AGAIN -- and an + // Abort that refunds a reagent, releases a lock or notifies a player does it + // twice, with nothing in the event's own code to suggest it could happen. + // ============================ + EventProcessor processor; + int aborts = 0; + + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, false)), 10); + + processor.KillAllEvents(false); + CHECK(aborts == 1); + CHECK(!processor.IsEmpty()); // undeletable, so it stayed queued + + processor.Update(100); // now due; destroyed here + CHECK(aborts == 1); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_ForcedKillFromInsideANonForcedKillDoesNotCorruptTheQueue) +{ + // Abort is virtual and runs game code, and that code may destroy the whole + // queue. A non-forced kill that walks m_events in place is holding an + // iterator into a container the inner call swaps out from under it. + EventProcessor processor; + int aborts = 0; + + processor.AddEvent(std::unique_ptr(new AbortKillsEvent(processor, true)), 10); + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, true)), 20); + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, false)), 30); + + processor.KillAllEvents(false); + + // Both siblings were aborted, once each, and nothing was walked after the + // container it lived in had been swapped away. + CHECK(aborts == 2); + + // ===== AND THE FORCE IS NOT DOWNGRADED ===== + // + // The undeletable sibling would ordinarily survive a non-forced kill. A + // handler asked for a FORCED one, and that request found an empty queue + // because the outer pass had already moved it aside -- so the outer pass is + // what has to honour it. Re-queueing the survivor here would mean "force" + // silently became "not force" whenever it arrived from an Abort. + // =========================================== + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_ForceRequestDoesNotLeakIntoTheNextKill) +{ + // The flag above must not outlive the call that raised it: a later, + // unrelated non-forced kill would then drop survivors nobody asked to drop. + EventProcessor processor; + int aborts = 0; + + processor.AddEvent(std::unique_ptr(new AbortKillsEvent(processor, true)), 10); + processor.KillAllEvents(true); + CHECK(processor.IsEmpty()); + + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, false)), 20); + processor.KillAllEvents(false); + + CHECK(aborts == 1); + CHECK(!processor.IsEmpty()); // undeletable, and no force was requested +} + +TEST(EventProcessor_NestedKillDoesNotClearTheGuardEarly) +{ + // An inner KillAllEvents that finishes by storing `false` leaves the OUTER + // call delivering aborts with the guard down. The probe has to be an Abort + // that tries to queue something, ordered AFTER the nested killer -- by then + // the inner call has returned, which is exactly the open window. + EventProcessor processor; + int accepted = 0; + int fires = 0; + int aborts = 0; + + processor.AddEvent(std::unique_ptr(new AbortKillsEvent(processor, true)), 100); + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, true)), 200); + processor.AddEvent( + std::unique_ptr(new AbortAddsEvent(processor, accepted, fires)), 300); + + processor.KillAllEvents(true); + + CHECK(processor.IsEmpty()); + CHECK(aborts == 1); + CHECK(accepted == 0); + CHECK(fires == 0); +} + +TEST(EventProcessor_SecondRescheduleOfTheSameEventIsRefused) +{ + // Two adoptions of one object put two unique_ptrs on it; the second + // destruction is a double free. The refusal is what makes returning "the + // queue owns me" safe for the caller. + EventProcessor processor; + bool secondAccepted = true; + + processor.AddEvent( + std::unique_ptr(new DoubleRescheduleEvent(processor, secondAccepted)), 0); + processor.Update(1); + + CHECK(!secondAccepted); + + // Destroying the processor here is itself the check: with two owners this + // would double-free. + processor.KillAllEvents(true); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_SameTickRescheduleIsNotAnUnboundedLoop) +{ + // Re-adding for the instant being processed must be deferred to the next + // pass. Picking it up again immediately makes Update never return, which on + // a live server is a hung world thread rather than a crash. + EventProcessor processor; + int fires = 0; + + processor.AddEvent(std::unique_ptr(new SameTickEvent(processor, fires)), 0); + processor.Update(1); + + CHECK(fires == 1); + CHECK(!processor.IsEmpty()); + + processor.Update(1); + CHECK(fires == 2); +} + +// ===== 3. Lifetime ===== + +TEST(EventProcessor_DestructorAbortsAndDestroysWhatIsStillQueued) +{ + int aborts = 0; + int deletableAborts = 0; + + { + EventProcessor processor; + processor.AddEvent(std::unique_ptr(new AbortCountingEvent(aborts, false)), 10); + processor.AddEvent( + std::unique_ptr(new AbortCountingEvent(deletableAborts, true)), 20); + } + + // Undeletable is not un-destroyable: a forced teardown takes everything. + CHECK(aborts == 1); + CHECK(deletableAborts == 1); +} + +TEST(EventProcessor_AbortRequestedEventIsNotExecuted) +{ + EventProcessor processor; + int fires = 0; + + auto event = std::unique_ptr(new CountingEvent(fires, 1)); + BasicEvent* raw = event.get(); + processor.AddEvent(std::move(event), 0); + raw->RequestAbort(); + + processor.Update(1); + + CHECK(fires == 0); + CHECK(processor.IsEmpty()); +} + +// ===== 4. Ordering, and the one happy path ===== + +TEST(EventProcessor_RunsInDueOrderAndOnlyWhatIsDue) +{ + EventProcessor processor; + int early = 0; + int late = 0; + + processor.AddEvent(std::unique_ptr(new CountingEvent(early, 1)), 10); + processor.AddEvent(std::unique_ptr(new CountingEvent(late, 1)), 100); + + processor.Update(50); + CHECK(early == 1); + CHECK(late == 0); + CHECK(!processor.IsEmpty()); + + processor.Update(100); + CHECK(late == 1); + CHECK(processor.IsEmpty()); +} + +TEST(EventProcessor_CalculateTimeIsRelativeToTheProcessorClock) +{ + EventProcessor processor; + + CHECK(processor.CalculateTime(10) == 10); + processor.Update(40); + CHECK(processor.CalculateTime(10) == 50); +} From 9dc1655ff7667b571aaef8dd2abc905a23b248c7 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 01:59:08 +0300 Subject: [PATCH 3/9] Fix the arrival check in RoutedPointMovementGenerator Fixed: Unit::GetDistance does not exist -- WorldObject owns no spatial API. The check goes through the placement component: owner.Where().DistanceTo(m_dest). Same frame and the same bounding-radius subtraction, so the 10 yd threshold is unchanged. Co-Authored-By: Claude Opus 5 --- src/game/MotionGenerators/PointMovementGenerator.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/game/MotionGenerators/PointMovementGenerator.cpp b/src/game/MotionGenerators/PointMovementGenerator.cpp index a9db53f6a..21281a79d 100644 --- a/src/game/MotionGenerators/PointMovementGenerator.cpp +++ b/src/game/MotionGenerators/PointMovementGenerator.cpp @@ -87,7 +87,9 @@ Motion::MoveIntent RoutedPointMovementGenerator::Intent(Unit& owner, Motion::Mov { // Arrival is latched here rather than refusal, and the proximity test rejects a mover that // was frozen partway rather than actually arriving. See the header for both reasons. - if (status.arrived && owner.GetDistance(m_dest.x, m_dest.y, m_dest.z) < 10.0f) + // m_dest is in the mover's own frame, which is what Where() measures in, so this is a + // same-frame comparison and never composes one across a transport boundary. + if (status.arrived && owner.Where().DistanceTo(m_dest) < 10.0f) { m_arrived = true; } From 431157550e1c3bf7303c16d5024caded2b2da9d3 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 02:04:37 +0300 Subject: [PATCH 4/9] Run CI on every branch Implemented: push triggers on '**' instead of [master, united, devel], so any branch is built before it reaches a pull request. Implemented: paths-ignore on push for '**.md', 'extra/doc/**' and 'LICENSE'. Left alone: pull_request triggers. A same-repo PR is already covered by push, and paths-ignore on a required check would leave a docs-only PR waiting for a run that never starts. Co-Authored-By: Claude Opus 5 --- .github/workflows/core_codestyle.yml | 6 +++++- .github/workflows/core_linux_build.yml | 6 +++++- .github/workflows/core_windows_build.yml | 6 +++++- .github/workflows/docker_build.yml | 6 +++++- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/.github/workflows/core_codestyle.yml b/.github/workflows/core_codestyle.yml index c429f4a1d..9af11a1bf 100644 --- a/.github/workflows/core_codestyle.yml +++ b/.github/workflows/core_codestyle.yml @@ -5,7 +5,11 @@ permissions: on: push: - branches: [master, united, devel] + branches: ['**'] + paths-ignore: + - '**.md' + - 'extra/doc/**' + - 'LICENSE' pull_request: branches: [master, united] diff --git a/.github/workflows/core_linux_build.yml b/.github/workflows/core_linux_build.yml index ead192a63..d0dfea476 100644 --- a/.github/workflows/core_linux_build.yml +++ b/.github/workflows/core_linux_build.yml @@ -2,7 +2,11 @@ name: Linux Build (GCC + Clang) on: push: - branches: [ master, united, devel ] + branches: [ '**' ] + paths-ignore: + - '**.md' + - 'extra/doc/**' + - 'LICENSE' pull_request: branches: [ master, united ] diff --git a/.github/workflows/core_windows_build.yml b/.github/workflows/core_windows_build.yml index 258b1e223..87bd70bf2 100644 --- a/.github/workflows/core_windows_build.yml +++ b/.github/workflows/core_windows_build.yml @@ -2,7 +2,11 @@ name: Windows Build (MSVC) on: push: - branches: [ master, united, devel ] + branches: [ '**' ] + paths-ignore: + - '**.md' + - 'extra/doc/**' + - 'LICENSE' pull_request: branches: [ master, united ] diff --git a/.github/workflows/docker_build.yml b/.github/workflows/docker_build.yml index 7341a089e..596f5b363 100644 --- a/.github/workflows/docker_build.yml +++ b/.github/workflows/docker_build.yml @@ -2,7 +2,11 @@ name: Docker Build on: push: - branches: [ master, united, devel ] + branches: [ '**' ] + paths-ignore: + - '**.md' + - 'extra/doc/**' + - 'LICENSE' pull_request: branches: [ master, united ] From e8dd0883ba7da86ca8141384f641a39a5074110f Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 02:45:38 +0300 Subject: [PATCH 5/9] Compare the stale-pid target by inode, not by path Fixed: two flawfinder [5] (race) readlink findings, which CodeFactor gates on. The reported CWE-362/CWE-20 did not apply -- the paths are compared and never opened, and the buffers were zero-initialised and read one short, so they were NUL-terminated. Verified with flawfinder 2.0.19: level >= 3 clean afterwards, and stat is not flagged in its place. Fixed: comparing by path string answered the wrong question. An in-place upgrade keeps the path and changes the file; a hard link or a bind mount gives one file two paths. Device plus inode is the identity actually being tested. Fixed: PATH_MAX buffers and their truncation handling are gone with it, along with the now-unused . Co-Authored-By: Claude Opus 5 --- src/shared/Process/PosixProcess.cpp | 30 ++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp index bc7466fd2..63c4df91e 100644 --- a/src/shared/Process/PosixProcess.cpp +++ b/src/shared/Process/PosixProcess.cpp @@ -29,7 +29,6 @@ #include #include -#include #include #include #include @@ -147,26 +146,35 @@ bool WritePidFile(const std::string& path) bool IsSameExecutable(pid_t pid) { #ifdef __linux__ - char mine[PATH_MAX + 1] = {}; - char theirs[PATH_MAX + 1] = {}; + // ===== IDENTITY, NOT SPELLING ===== + // + // stat() through /proc//exe lands on the binary itself, so the two are + // compared by device and inode rather than by the path they happen to be + // reachable at. That is the question actually being asked, and the string + // form got it wrong in both directions: an in-place upgrade leaves the path + // identical while the file underneath is a different one, and a hard link or + // a bind mount gives one file two names. + // + // No buffer either, so there is no PATH_MAX and no truncation to reason about. + // ================================== + char link[64] = {}; + std::snprintf(link, sizeof(link), "/proc/%ld/exe", static_cast(pid)); - const ssize_t m = readlink("/proc/self/exe", mine, sizeof(mine) - 1); - if (m <= 0) + struct stat mine = {}; + struct stat theirs = {}; + + if (stat("/proc/self/exe", &mine) != 0) { return true; } - char link[64] = {}; - std::snprintf(link, sizeof(link), "/proc/%ld/exe", static_cast(pid)); - - const ssize_t t = readlink(link, theirs, sizeof(theirs) - 1); - if (t <= 0) + if (stat(link, &theirs) != 0) { // No such process, or not ours to look at. kill() answers that better. return true; } - return std::strcmp(mine, theirs) == 0; + return mine.st_dev == theirs.st_dev && mine.st_ino == theirs.st_ino; #else // FreeBSD and macOS need sysctl/libproc for this. Not worth the platform code // until a stale pid file actually bites somewhere other than Linux. From 4fd59abd318c575b8e680635488bd7bf6f8b3317 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 02:57:06 +0300 Subject: [PATCH 6/9] Address the Codex review findings on #480 Fixed: P1 -- a Windows service could not start. The SCM launches the process in system32, and main() looked for the configuration before RunInBackground, so the supported fallback beside the binary resolved in the wrong directory and main() returned before ServiceMain ever changed it. Process::UseExecutableDirectory() now runs first for -s run; a no-op on POSIX, which keeps the invocation directory on purpose. Fixed: P1 -- Stop() accepted a pid outside pid_t. long 4294967295 passed the "> 0" test and narrowed to pid_t(-1); IsSameExecutable() reads /proc/-1/exe as "cannot say", and kill(-1, SIGINT) then signals every process the caller owns. Both ends of the range are checked before either cast. Fixed: P1 -- use-after-free in Spell::prepare. A triggered cast started from inside KillAllEvents is refused by AddEvent, which aborts and destroys the SpellEvent, and ~SpellEvent cancels and deletes the Spell still executing prepare(). The return value is checked and prepare() leaves without touching a member. Fixed: P2 -- the forked child inherited SIGINT/SIGTERM pointed at the parent's startup handler, which returns without acting in the child and so swallowed both until Serve() installed the server's own. They are reset to SIG_DFL while still blocked, and unblocked only afterwards. Co-Authored-By: Claude Opus 5 --- src/game/WorldHandlers/Spell.cpp | 16 ++++++++-- src/mangosd/mangosd.cpp | 11 +++++++ src/shared/Process/PosixProcess.cpp | 43 +++++++++++++++++++++------ src/shared/Process/Process.h | 15 ++++++++++ src/shared/Process/WindowsProcess.cpp | 20 +++++++++++++ 5 files changed, 94 insertions(+), 11 deletions(-) diff --git a/src/game/WorldHandlers/Spell.cpp b/src/game/WorldHandlers/Spell.cpp index de7509b98..517b96848 100644 --- a/src/game/WorldHandlers/Spell.cpp +++ b/src/game/WorldHandlers/Spell.cpp @@ -599,8 +599,20 @@ SpellCastResult Spell::prepare(SpellCastTargets const* targets, Aura* triggeredB } // create and add update event for this spell - m_caster->m_Events.AddEvent(std::unique_ptr(new SpellEvent(this)), - m_caster->m_Events.CalculateTime(1)); + if (!m_caster->m_Events.AddEvent(std::unique_ptr(new SpellEvent(this)), + m_caster->m_Events.CalculateTime(1))) + { + // Refused, which means the caster's processor is tearing down -- an aura + // removal or a cancellation can start a triggered cast from inside + // KillAllEvents. AddEvent then aborted and destroyed the event, and + // ~SpellEvent cancelled and deleted THIS Spell on the way out. + // + // So nothing below may run and no member may be touched, not even to + // report the failure: `this` is already freed. Every caller drops the + // pointer after prepare() and lets the event own the spell, which is why + // returning here is the whole of the cleanup. + return SPELL_FAILED_DONT_REPORT; + } // Prevent casting at cast another spell (ServerSide check) if (!m_IsTriggeredSpell && m_caster->IsNonMeleeSpellCasted(false, true, true)) diff --git a/src/mangosd/mangosd.cpp b/src/mangosd/mangosd.cpp index f41c59370..ef7621b04 100644 --- a/src/mangosd/mangosd.cpp +++ b/src/mangosd/mangosd.cpp @@ -414,6 +414,17 @@ int main(int argc, char **argv) break; } + // Before the configuration is looked for, not after. A Windows service is + // started from system32, so the fallback below -- the config file beside the + // binary -- would be searched for in the wrong directory and the service + // would fail to start before RunInBackground ever got the chance to correct + // it. A no-op everywhere else. + if (action == Process::ServiceAction::Run && !Process::UseExecutableDirectory()) + { + Log::WaitBeforeContinueIfNeed(); + return 1; + } + if (!sConfig.SetSource(cfg_file)) { // Try current folder as fallback if SYSCONFDIR path fails diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp index 63c4df91e..f8d67283e 100644 --- a/src/shared/Process/PosixProcess.cpp +++ b/src/shared/Process/PosixProcess.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -190,6 +191,14 @@ bool HasServiceManager() return false; } +bool UseExecutableDirectory() +{ + // Nothing to move to. A POSIX start keeps the directory it was invoked from + // until RunInBackground forks, and that is what makes a relative -c path and + // a relative pid file mean what the operator typed. + return true; +} + bool Install(const Options&) { sLog.outError("This platform has no service manager to install into." @@ -232,10 +241,9 @@ int RunInBackground(const Options& options, const std::function& serve) const pid_t child = fork(); g_childPid = child; - sigprocmask(SIG_SETMASK, &previous, nullptr); - if (child < 0) { + sigprocmask(SIG_SETMASK, &previous, nullptr); sLog.outError("Cannot fork into the background: %s", std::strerror(errno)); return EXIT_FAILURE; } @@ -245,6 +253,8 @@ int RunInBackground(const Options& options, const std::function& serve) // The parent. It waits here until the child reports ready, dies, or -- // where one was asked for -- the alarm fires; every one of those leaves // through the handler. + sigprocmask(SIG_SETMASK, &previous, nullptr); + if (options.readyTimeoutSeconds != 0) { alarm(options.readyTimeoutSeconds); @@ -259,13 +269,24 @@ int RunInBackground(const Options& options, const std::function& serve) } } - // The child. Its inherited handlers are for the parent's wait, not for it. - // SIGCHLD back to default matters: the server forks nothing, but leaving a - // parent's handler installed is how an inherited disposition surprises the - // next person who adds a helper process. + // The child. Its inherited handlers belong to the parent's wait, and in the + // child HandleStartupSignal returns without doing anything -- so leaving + // SIGINT and SIGTERM pointed at it would swallow them for the whole of + // start-up, until Serve() installs the server's own. A start command + // cancelled in that window, or a `-s stop` racing it, would be ignored and + // the daemon would survive. + // + // Reset while the two are still blocked, and unblocked only afterwards, so + // there is no instant where the disposition is the parent's and the signal + // can arrive. SIGCHLD goes back too: the server forks nothing today, but an + // inherited disposition is how that surprises whoever adds a helper process. std::signal(SIGUSR1, SIG_DFL); std::signal(SIGALRM, SIG_DFL); std::signal(SIGCHLD, SIG_DFL); + std::signal(SIGINT, SIG_DFL); + std::signal(SIGTERM, SIG_DFL); + + sigprocmask(SIG_SETMASK, &previous, nullptr); umask(0); @@ -324,10 +345,14 @@ bool Stop(const Options& options) } long pid = 0; - if (!(file >> pid) || pid <= 0) + if (!(file >> pid) || pid <= 0 || + pid > static_cast(std::numeric_limits::max())) { - // A pid of zero or below would go to the process GROUP or to every - // process this user owns. Refused rather than sent. + // Zero or below would go to the process GROUP or to every process this + // user owns. The upper bound matters just as much and is easier to miss: + // pid_t is 32-bit where long is 64-bit, so 4294967295 passes a "> 0" + // test and then narrows to -1 -- and kill(-1) is the same broadcast by + // another route. Both ends are checked before either cast. sLog.outError("The pid file %s does not name a process", options.pidFile.c_str()); return false; } diff --git a/src/shared/Process/Process.h b/src/shared/Process/Process.h index 02ec29b51..65e3740af 100644 --- a/src/shared/Process/Process.h +++ b/src/shared/Process/Process.h @@ -102,6 +102,21 @@ struct Options /// POSIX, and that is not a gap: init is not ours to write into. bool HasServiceManager(); +/** + * @brief Move to the directory the executable lives in. + * + * Call before reading anything by a relative path in a background start. The + * Windows service manager launches the process from system32, so the + * configuration file beside the binary -- and every relative path inside it -- + * resolves somewhere else until this has run. RunInBackground() does it again + * once the service thread is up, but that is too late for a caller that has to + * read its configuration first in order to know its pid file. + * + * A no-op returning true on POSIX, where the process keeps the directory it was + * started from until it forks. + */ +bool UseExecutableDirectory(); + /// Register with the service manager. False, with a reason logged, where there /// is none. bool Install(const Options& options); diff --git a/src/shared/Process/WindowsProcess.cpp b/src/shared/Process/WindowsProcess.cpp index 8a2f48823..470cc5acd 100644 --- a/src/shared/Process/WindowsProcess.cpp +++ b/src/shared/Process/WindowsProcess.cpp @@ -201,6 +201,26 @@ bool HasServiceManager() return true; } +bool UseExecutableDirectory() +{ + const std::string directory = ExecutableDirectory(); + + if (directory.empty()) + { + sLog.outError("SERVICE: cannot determine this executable's directory."); + return false; + } + + if (!SetCurrentDirectoryA(directory.c_str())) + { + sLog.outError("SERVICE: cannot change to '%s': error %u", + directory.c_str(), unsigned(GetLastError())); + return false; + } + + return true; +} + bool Install(const Options& options) { SC_HANDLE manager = OpenSCManagerA(nullptr, nullptr, SC_MANAGER_CREATE_SERVICE); From e5fe5db60eda6785be0da33bbb6e46b207402ef5 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 03:11:15 +0300 Subject: [PATCH 7/9] Address the Codacy findings on #480 Fixed: umask(0) in the forked child (CWE-732). The pid file and the logs are created through streams asking for 0666, so a zero mask made them world-writable -- including the pid file that `-s stop` trusts. Now 077; group or world access is a deployment decision, set from outside (systemd UMask=). Fixed: RedirectStandardStreams discarded three freopen results into a short-circuited boolean. Each is stored and checked, and all three are attempted rather than short-circuited: one failure is no reason to leave the other two pointed at a terminal that is going away. Fixed: '!IsSameExecutable(...)' was always false off Linux, because the function was stubbed to return true there. It is now defined only where it can be answered and the caller drops the check entirely elsewhere -- a guard whose condition is unreachable reads as a guard and is not one. Fixed: the discarded unique_ptr::release() in EventProcessor::Update is now an explicit static_cast. Ownership moves to whichever queue adopted the event; dropping the pointer is the intent, not an oversight. Co-Authored-By: Claude Opus 5 --- src/shared/Process/PosixProcess.cpp | 65 +++++++++++++++---------- src/shared/Utilities/EventProcessor.cpp | 2 +- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp index f8d67283e..12ff25940 100644 --- a/src/shared/Process/PosixProcess.cpp +++ b/src/shared/Process/PosixProcess.cpp @@ -115,9 +115,16 @@ bool RedirectStandardStreams() // The daemon has no terminal. Left attached, the first write to stdout after // the terminal closes takes the process down with SIGHUP or a write error at // an arbitrary moment. - return std::freopen("/dev/null", "rt", stdin) - && std::freopen("/dev/null", "wt", stdout) - && std::freopen("/dev/null", "wt", stderr); + // + // freopen hands back the stream it was given, not a new one, so there is + // nothing here to own or close -- only success to check. All three are + // attempted rather than short-circuited: a failure on one is no reason to + // leave the other two pointed at a terminal that is going away. + FILE* const in = std::freopen("/dev/null", "rt", stdin); + FILE* const out = std::freopen("/dev/null", "wt", stdout); + FILE* const err = std::freopen("/dev/null", "wt", stderr); + + return in != nullptr && out != nullptr && err != nullptr; } bool WritePidFile(const std::string& path) @@ -137,27 +144,27 @@ bool WritePidFile(const std::string& path) return bool(file); } +#ifdef __linux__ + /** * @brief Is @p pid running the same executable as this process? * - * @return true when it is, AND when the platform cannot say. A false negative - * would refuse a legitimate stop, which is worse than the stale-pid case - * this guards -- so only a definite mismatch refuses. + * Defined only where it can be answered. Elsewhere the caller drops the check + * entirely rather than calling a function that can only say yes -- a guard whose + * condition is unreachable reads as a guard, and is not one. + * + * @return true when it is, and when the answer cannot be obtained. A false + * negative would refuse a legitimate stop, which is worse than the + * stale-pid case this guards, so only a definite mismatch refuses. + * + * stat() through /proc//exe lands on the binary itself, so the two are + * compared by device and inode rather than by the path they happen to be + * reachable at. The string form got that wrong in both directions: an in-place + * upgrade leaves the path identical while the file underneath is a different + * one, and a hard link or a bind mount gives one file two names. */ bool IsSameExecutable(pid_t pid) { -#ifdef __linux__ - // ===== IDENTITY, NOT SPELLING ===== - // - // stat() through /proc//exe lands on the binary itself, so the two are - // compared by device and inode rather than by the path they happen to be - // reachable at. That is the question actually being asked, and the string - // form got it wrong in both directions: an in-place upgrade leaves the path - // identical while the file underneath is a different one, and a hard link or - // a bind mount gives one file two names. - // - // No buffer either, so there is no PATH_MAX and no truncation to reason about. - // ================================== char link[64] = {}; std::snprintf(link, sizeof(link), "/proc/%ld/exe", static_cast(pid)); @@ -176,14 +183,10 @@ bool IsSameExecutable(pid_t pid) } return mine.st_dev == theirs.st_dev && mine.st_ino == theirs.st_ino; -#else - // FreeBSD and macOS need sysctl/libproc for this. Not worth the platform code - // until a stale pid file actually bites somewhere other than Linux. - (void)pid; - return true; -#endif } +#endif // __linux__ + } // namespace bool HasServiceManager() @@ -288,7 +291,12 @@ int RunInBackground(const Options& options, const std::function& serve) sigprocmask(SIG_SETMASK, &previous, nullptr); - umask(0); + // Not umask(0). The daemon creates its pid file and its logs through streams + // that ask for 0666, so a zero mask hands both to every user on the box -- + // world-writable pid file included, which is the one file `-s stop` trusts. + // Owner-only; a deployment that wants group access sets it from the outside + // (systemd UMask=), where that decision belongs. + umask(077); if (setsid() < 0) { @@ -359,8 +367,10 @@ bool Stop(const Options& options) // A pid file outlives the process it names, and pids are reused. Signalling // whatever now holds that number is how a stale file comes to interrupt an - // unrelated program. Where the check is available, the target must be running - // the same executable as this one. + // unrelated program, so on Linux the target must be running this same + // executable. FreeBSD and macOS would need sysctl or libproc; until a stale + // pid file bites there, the check is absent rather than stubbed out to yes. +#ifdef __linux__ if (!IsSameExecutable(static_cast(pid))) { sLog.outError("The pid file %s names process %ld, which is not this server;" @@ -368,6 +378,7 @@ bool Stop(const Options& options) options.pidFile.c_str(), pid); return false; } +#endif if (kill(static_cast(pid), SIGINT) < 0) { diff --git a/src/shared/Utilities/EventProcessor.cpp b/src/shared/Utilities/EventProcessor.cpp index cb4707956..47ee0cd20 100644 --- a/src/shared/Utilities/EventProcessor.cpp +++ b/src/shared/Utilities/EventProcessor.cpp @@ -97,7 +97,7 @@ void EventProcessor::Update(std::uint32_t elapsed) // That is the contract's one sharp edge and it cannot be detected // from here -- the event may legitimately have gone to a processor // this one has never heard of. - event.release(); + static_cast(event.release()); } } } From 28a7de371e74befcf362cbe74d85a8e69121c955 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 03:26:56 +0300 Subject: [PATCH 8/9] Keep the redirected streams as outcomes, not as pointers Fixed: storing the freopen results in FILE* variables traded one Codacy finding for another -- a resource leak, since nothing closes them. freopen returns the stream it was handed, so the pointer must not be kept; only the success of each call is. cppcheck reports no leak on the result. Note: the umask finding stays. flawfinder flags the call unconditionally, not the value, and the value is already 077 -- the most restrictive it suggests. It can only be silenced by not setting a umask at all, which would inherit the operator shell default and hand world-readable logs back. Co-Authored-By: Claude Opus 5 --- src/shared/Process/PosixProcess.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp index 12ff25940..27999d0a5 100644 --- a/src/shared/Process/PosixProcess.cpp +++ b/src/shared/Process/PosixProcess.cpp @@ -116,15 +116,17 @@ bool RedirectStandardStreams() // the terminal closes takes the process down with SIGHUP or a write error at // an arbitrary moment. // - // freopen hands back the stream it was given, not a new one, so there is - // nothing here to own or close -- only success to check. All three are - // attempted rather than short-circuited: a failure on one is no reason to - // leave the other two pointed at a terminal that is going away. - FILE* const in = std::freopen("/dev/null", "rt", stdin); - FILE* const out = std::freopen("/dev/null", "wt", stdout); - FILE* const err = std::freopen("/dev/null", "wt", stderr); - - return in != nullptr && out != nullptr && err != nullptr; + // Only the outcome is kept, never the pointer: freopen hands back the stream + // it was given, not a new one, so there is nothing here to own and closing it + // would close the standard stream itself. + // + // All three are attempted rather than short-circuited -- one failure is no + // reason to leave the other two pointed at a terminal that is going away. + const bool okIn = std::freopen("/dev/null", "rt", stdin) != nullptr; + const bool okOut = std::freopen("/dev/null", "wt", stdout) != nullptr; + const bool okErr = std::freopen("/dev/null", "wt", stderr) != nullptr; + + return okIn && okOut && okErr; } bool WritePidFile(const std::string& path) From 2b20d3da658183b3f293dc4990720baea948f999 Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 21 Aug 2026 08:45:03 +0300 Subject: [PATCH 9/9] Address the second round of Codex findings on #480 Fixed: P2 -- `mangosd -s stop` failed on Windows when run from any directory but the executable's. The stop was dispatched only after the configuration had been loaded, so an installation relying on the mangosd.conf fallback beside the binary returned on the config lookup and never reached Process::Stop(). Where there is a service manager the stop needs nothing but the service name, so it is dispatched before the configuration is looked for; without one the pid file is the only handle on the instance, so the POSIX stop still falls through to after the load. Fixed: P2 -- SIGCHLD was installed with std::signal, which leaves SA_NOCLDSTOP clear, so the parent was also woken when the child merely STOPPED. The handler reads every SIGCHLD as death, so a debugger or a supervisor pausing the child made the start command report failure while leaving a stopped child that finishes daemonising on SIGCONT. Installed through sigaction with SA_NOCLDSTOP, so only a real exit arrives. Co-Authored-By: Claude Opus 5 --- src/mangosd/mangosd.cpp | 16 ++++++++++++++++ src/shared/Process/PosixProcess.cpp | 13 ++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/mangosd/mangosd.cpp b/src/mangosd/mangosd.cpp index ef7621b04..fc258b550 100644 --- a/src/mangosd/mangosd.cpp +++ b/src/mangosd/mangosd.cpp @@ -410,6 +410,22 @@ int main(int argc, char **argv) case Process::ServiceAction::Uninstall: return Process::Uninstall(processOptions) ? 0 : 1; + case Process::ServiceAction::Stop: + // Where there is a service manager, stopping is its business and needs + // nothing but the service name -- so it must not be made to depend on + // finding a configuration file. `mangosd -s stop` run from any other + // directory would otherwise fail on the config lookup below and never + // reach the stop it was asked for. + // + // Without one, the pid file is the only handle on the running instance + // and that comes from the configuration, so the POSIX stop falls through + // and is dispatched after it is loaded. + if (Process::HasServiceManager()) + { + return Process::Stop(processOptions) ? 0 : 1; + } + break; + default: break; } diff --git a/src/shared/Process/PosixProcess.cpp b/src/shared/Process/PosixProcess.cpp index 27999d0a5..574f48221 100644 --- a/src/shared/Process/PosixProcess.cpp +++ b/src/shared/Process/PosixProcess.cpp @@ -227,11 +227,22 @@ int RunInBackground(const Options& options, const std::function& serve) g_parentPid = getpid(); std::signal(SIGUSR1, HandleStartupSignal); - std::signal(SIGCHLD, HandleStartupSignal); std::signal(SIGINT, HandleStartupSignal); std::signal(SIGTERM, HandleStartupSignal); std::signal(SIGALRM, HandleStartupSignal); + // SIGCHLD through sigaction, for SA_NOCLDSTOP alone. std::signal leaves it + // clear, and SIGCHLD is then delivered when the child merely STOPS as well as + // when it dies -- a debugger, a supervisor or a plain ^Z on the child. The + // handler reads every SIGCHLD as death, so the parent would report a failed + // start and exit while leaving a stopped child that finishes daemonising the + // moment it is continued. With the flag, only a real exit arrives here. + struct sigaction childAction = {}; + childAction.sa_handler = HandleStartupSignal; + sigemptyset(&childAction.sa_mask); + childAction.sa_flags = SA_NOCLDSTOP; + sigaction(SIGCHLD, &childAction, nullptr); + // Blocked across the fork. g_childPid is assigned only AFTER fork returns, // so a SIGTERM landing in that window would find it still zero: the handler // would forward nothing and the parent would leave a child running with no