Coeherent Win/Posix service management and fixed EventProcessor critical defects - #480
Coeherent Win/Posix service management and fixed EventProcessor critical defects #480H0zen wants to merge 9 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 158 complexity · 0 duplication
Metric Results Complexity 158 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@codex review |
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 <climits>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 431157550e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 <noreply@anthropic.com>
|
@codex re-review the latest commit does it fix the findings in the earlier review you did. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fd59abd31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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<void>. Ownership moves to whichever queue adopted the event; dropping the pointer is the intent, not an oversight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
@codex re-review the latest commit does it fix the findings in the earlier review you did. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28a7de371e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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()) |
There was a problem hiding this comment.
When mangosd.exe -s stop is invoked outside the executable directory and the installation relies on the supported local mangosd.conf fallback, this condition still changes directories only for Run; configuration loading at lines 428–435 therefore fails before the Windows Process::Stop() call, even though that implementation needs only the fixed service name. Fresh evidence in the reviewed tree is that the attempted directory fix remains explicitly restricted to ServiceAction::Run; include Stop or dispatch the Windows stop before configuration loading.
Useful? React with 👍 / 👎.
| g_parentPid = getpid(); | ||
|
|
||
| std::signal(SIGUSR1, HandleStartupSignal); | ||
| std::signal(SIGCHLD, HandleStartupSignal); |
There was a problem hiding this comment.
When the daemon child receives SIGSTOP during startup, the parent receives SIGCHLD, but this installation still lacks SA_NOCLDSTOP and the handler unconditionally treats every SIGCHLD as child death. The start command consequently exits with failure while leaving a merely stopped child that can finish daemonizing after SIGCONT. Fresh evidence in the reviewed tree is that SIGCHLD is still installed via std::signal with no waitpid status check; install it with SA_NOCLDSTOP or verify that the child actually exited.
Useful? React with 👍 / 👎.
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 <noreply@anthropic.com>
3 participants
This change is