Repository navigation
mpm_event: Fix just_die() calling non-async-signal-safe code. - #967
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
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
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.
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.
Reported from a real CI environment: DataDog/dd-trace-php#4304, with a backtrace showing
just_die->exit(0)interruptingapr_allocator_create->calloc.Reproduced the underlying mechanism directly (not a simulation): a small standalone harness with 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 artificially widening the window, 300/300 trials came back clean, since that critical section is only nanoseconds wide -- 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 already used for 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.
BZ: https://bz.apache.org/bugzilla/show_bug.cgi?id=70267
This PR does not report or address a security vulnerability
(see
SECURITY.mdandhttps://www.apache.org/security/#reporting-a-vulnerability).
Code follows the httpd style guide.
New log messages (level debug or higher) use an empty
APLOGNO()tag;numbers are assigned by a committer at merge time (see
docs/log-message-tags/README).Not applicable -- no new log messages.
If the change is user-visible, a
changes-entries/*.txtfile is included,following the template in
README.CHANGES(not needed otherwise).Test cases based on pyhttpd are included where appropriate, in the
test/modules/xxxdirectory matching themodules/xxxmodule source,or
test/modules/corefor core server changes. On Unix, verify thesepass locally with e.g.
make check-pytest PYTEST_DIRS=test/modules/xxx.Not included: this is a signal-delivery-timing race during child-process startup, not something the pytest harness's HttpdConf/env abstractions can control or trigger deterministically. Verified instead with the standalone harness reproduction described above, which directly exercises the same mechanism as the real code (confirmed by diffing the harness's handler against just_die()/clean_child_exit()'s actual call shape).