Repository navigation
mpm_event: Fix just_die() calling non-async-signal-safe code. (#967) - #1020
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
…#967) just_die() is the SIGTERM handler installed in a child immediately after fork(), before child_main() has done its allocator-heavy early startup work. It called clean_child_exit(), which runs hook callbacks, calls apr_pool_destroy() (more malloc/free), and exit() (also not async-signal-safe, e.g. it may run atexit handlers that allocate). A signal handler runs on the thread it interrupts, so if that thread was itself inside malloc() when the signal arrived, this could re-enter the allocator on the same thread while it was already in progress, which is not safe and can deadlock or corrupt it. Reproduced the underlying mechanism directly: a SIGTERM handler that mirrors just_die()'s exact shape (take the same lock the interrupted thread may hold, do real malloc/free work, then exit()) self-deadlocks 20/20 times when the handler and the interrupted thread contend for the same lock, and 20/20 clean when the handler just calls _exit() instead, matching the fix below. Against the real glibc allocator without widening the window, 300/300 trials came back clean, since that critical section is only nanoseconds wide and far narrower than the scheduling latency of an externally delivered signal -- which is also why this surfaces rarely in the wild (reported from a CI environment where added instrumentation apparently widened the real window enough to hit it) rather than routinely. This window closes once child_main() installs its real, async-signal- safe SIGTERM handling (a no-op handler that only unblocks a syscall, with the actual shutdown processed synchronously outside signal context, same pattern as the listener thread's own wakeup signal). just_die() only needs to cover the brief gap before that, when nothing meaningful has been set up yet to justify running hook callbacks or pool cleanup. Just exit immediately instead. PR 70267 (cherry picked from commit 470b195)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(cherry picked from commit 470b195)
Backport of #967.
BZ: https://bz.apache.org/bugzilla/show_bug.cgi?id=70267
[x] This PR does not report or address a security vulnerability (see SECURITY.md and https://www.apache.org/security/#reporting-a-vulnerability).
[x] Code follows the httpd style guide.
[x] New log messages (level debug or higher) use an empty APLOGNO() tag (not applicable -- no new log messages).
[x] If the change is user-visible, a changes-entries/*.txt file is included (carried over unchanged from the trunk commit).
[x] Test cases based on pyhttpd are included where appropriate (not applicable -- this is a signal-delivery-timing race during child-process startup, not something the pytest harness can trigger deterministically; same rationale as the original PR #967. Verified instead by rebuilding this exact 2.4.x worktree and confirming at the disassembly level that just_die() now calls _exit() directly instead of clean_child_exit()).