mirror of
https://github.com/brazilofmux/tinymux
synced 2026-08-13 00:23:11 -04:00
fix: avoid fork() deadlock from alarm mutex in helper children
The alarm clock (the runaway-softcode/PCRE CPU limiter) was migrated from a SIGPROF interval timer to a std::thread + condition_variable, but the post-fork children still called alarm_clock.clear(), which locks mux_alarm::mutex_. That mutex is held by the alarm thread on every command via set()/clear(). fork() duplicates only the calling thread, so the alarm thread does not exist in the child and the alarm can never fire there. If fork() lands in the alarm thread's brief mutex window, the child inherits mutex_ locked with no owner and self-deadlocks in clear() before exec. A later stubslave reboot/shutdown then blocks the parent forever in waitpid(pid, NULL, 0), parking the entire network loop: idle, no CPU, not accepting connections, reconnect gets nothing, and only a full restart clears it. The rare race plus the need for a subsequent slave reboot explains why it took days. Fixes: - Child side (root cause): replace alarm_clock.clear() with the lock-free, async-signal-safe alarm_clock.alarmed.store(false) in the three fork children (stubslave boot, database dump, helper process). - Defense in depth: add reap_child_bounded() (WNOHANG poll, then SIGKILL) in place of the two blocking waitpid() calls in the stubslave paths, so no wedged child can ever park the main thread again. This also cleans up the orphaned stubslaves that accumulated across restarts. The legitimate main-thread alarm_clock.clear() calls in net.cpp and cque.cpp (which arm/disarm the CPU limiter around command execution) are unchanged. Pending field validation on the farm test deployment; do not push yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
eb93309f6a
commit
0c413ffcaa
4 changed files with 60 additions and 11 deletions
|
|
@ -39,6 +39,20 @@ performance work and a large expansion of the smoke-test suite.
|
|||
|
||||
## Reliability and Restart
|
||||
|
||||
- A `fork()`-with-locked-mutex deadlock that could hang the server after
|
||||
days of uptime is fixed. The alarm clock (the runaway-softcode/PCRE CPU
|
||||
limiter) is a `std::thread` plus condition variable whose mutex is taken
|
||||
briefly on every command. The stubslave, database-dump, and helper-
|
||||
process children each called `alarm_clock.clear()` immediately after
|
||||
`fork()`; since `fork()` copies only the calling thread, a child that
|
||||
forked during the alarm thread's mutex window inherited that mutex locked
|
||||
with no owner and self-deadlocked in `clear()` before `exec`. A later
|
||||
stubslave reboot then blocked the parent forever in `waitpid()`, parking
|
||||
the whole network loop — idle, no CPU, refusing new connections — until a
|
||||
manual restart. The children now reset the lock-free `alarmed` flag
|
||||
instead of taking the mutex, and the parent's blocking `waitpid()` calls
|
||||
are bounded (poll, then `SIGKILL`) so a wedged child can never stall the
|
||||
server.
|
||||
- A connect-time crash loop is fixed. A player whose `A_LOGINDATA` was
|
||||
truncated or malformed made `decrypt_logindata()` dereference a null
|
||||
field pointer and `SIGSEGV` on connect; and, separately,
|
||||
|
|
|
|||
|
|
@ -1767,10 +1767,12 @@ void fork_and_dump(int key)
|
|||
}
|
||||
if (child == 0)
|
||||
{
|
||||
// If we don't clear this alarm, the child will eventually receive a
|
||||
// SIG_PROF.
|
||||
//
|
||||
alarm_clock.clear();
|
||||
// In the forked child the alarm thread does not exist, so the alarm
|
||||
// can never fire here. Only reset the lock-free, async-signal-safe
|
||||
// flag so the dump runs unabbreviated. Do NOT call
|
||||
// alarm_clock.clear() — it locks a std::mutex that may have been
|
||||
// inherited locked across fork(), deadlocking the dump child.
|
||||
alarm_clock.alarmed.store(false);
|
||||
#endif // HAVE_WORKING_FORK
|
||||
|
||||
if (key & DUMP_STRUCT)
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@
|
|||
#if defined(__linux__)
|
||||
#include <dirent.h> // count_open_fds() reads /proc/self/fd
|
||||
#endif
|
||||
#include <csignal> // kill(), SIGKILL — bounded child reaping
|
||||
#endif
|
||||
|
||||
#if defined(_WIN32)
|
||||
|
|
@ -3159,6 +3160,32 @@ extern QUEUE_INFO Queue_In;
|
|||
extern QUEUE_INFO Queue_Out;
|
||||
extern pid_t stubslave_pid;
|
||||
|
||||
// Reap a forked child without ever blocking the main thread forever. A child
|
||||
// that wedges before exec would otherwise park us in waitpid() with no timeout,
|
||||
// stalling the entire network loop (idle, no CPU, not accepting). Poll briefly,
|
||||
// then force the issue with SIGKILL (guaranteed to terminate a futex-blocked
|
||||
// process) so a wedged child can never park the server.
|
||||
static void reap_child_bounded(pid_t pid)
|
||||
{
|
||||
if (pid <= 0)
|
||||
{
|
||||
return;
|
||||
}
|
||||
for (int i = 0; i < 50; i++) // ~5s grace at 100ms
|
||||
{
|
||||
pid_t r = waitpid(pid, nullptr, WNOHANG);
|
||||
if (r == pid || (r == -1 && errno == ECHILD))
|
||||
{
|
||||
return;
|
||||
}
|
||||
usleep(100000);
|
||||
}
|
||||
kill(pid, SIGKILL);
|
||||
while (waitpid(pid, nullptr, 0) == -1 && errno == EINTR)
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
bool GanlAdapter::boot_stubslave()
|
||||
{
|
||||
const char *pFailedFunc = nullptr;
|
||||
|
|
@ -3205,10 +3232,12 @@ bool GanlAdapter::boot_stubslave()
|
|||
|
||||
case 0:
|
||||
|
||||
// If we don't clear this alarm, the child will eventually receive a
|
||||
// SIG_PROF.
|
||||
//
|
||||
alarm_clock.clear();
|
||||
// In the forked child the alarm thread does not exist, so the alarm can
|
||||
// never fire here. Only reset the lock-free, async-signal-safe flag.
|
||||
// Do NOT call alarm_clock.clear() — it locks a std::mutex that may have
|
||||
// been inherited locked across fork(), deadlocking the child before exec
|
||||
// (and then parking the parent forever in the waitpid() below).
|
||||
alarm_clock.alarmed.store(false);
|
||||
|
||||
// Child. The following calls to dup2() assume only the minimal
|
||||
// dup2() functionality. That is, the destination descriptor is
|
||||
|
|
@ -3253,7 +3282,7 @@ failure:
|
|||
|
||||
if (stubslave_pid > 0)
|
||||
{
|
||||
waitpid(stubslave_pid, nullptr, 0);
|
||||
reap_child_bounded(stubslave_pid);
|
||||
}
|
||||
stubslave_pid = 0;
|
||||
|
||||
|
|
@ -3278,7 +3307,7 @@ void GanlAdapter::shutdown_stubslave()
|
|||
|
||||
if (stubslave_pid > 0)
|
||||
{
|
||||
waitpid(stubslave_pid, nullptr, 0);
|
||||
reap_child_bounded(stubslave_pid);
|
||||
}
|
||||
stubslave_pid = 0;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -208,7 +208,11 @@ MUX_RESULT CPlatform::BootHelperProcess(
|
|||
goto failure;
|
||||
|
||||
case 0:
|
||||
alarm_clock.clear();
|
||||
// Child: the alarm thread does not exist post-fork, so the alarm can
|
||||
// never fire here. Reset the lock-free flag only — do NOT call
|
||||
// alarm_clock.clear(), which locks a std::mutex possibly inherited
|
||||
// locked across fork() and would deadlock the child before exec.
|
||||
alarm_clock.alarmed.store(false);
|
||||
mux_close(sv[0]);
|
||||
if (sv[1] != 0)
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue