Fix _handle_signals TypeError when SIGTERM is delivered to a forked child - #70123
Open
dwoz wants to merge 2 commits into
Open
Fix _handle_signals TypeError when SIGTERM is delivered to a forked child#70123dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
signal.default_int_handler(signal.SIGTERM)(*args) called default_int_handler with a single positional argument (it requires (signum, frame)), raised TypeError, and killed the receiving process with an unhandled exception instead of triggering the intended KeyboardInterrupt-based clean shutdown. Observed in the wild on 3008.1's MasterPubServerChannel._publish_daemon when SIGTERM was delivered to the master. The daemon crashed and ProcessManager did not respawn it. The elif arm is reachable in the common case: signal.getsignal(SIGTERM) returns signal.Handlers.SIG_DFL, which is not callable and not None, so the first two arms fall through to the buggy call. Pass through the received (signum, frame) tuple; default_int_handler raises KeyboardInterrupt regardless of signum, matching the intent.
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.
What does this PR do?
Fixes a decade-old bug in
salt.utils.process.ProcessManager._handle_signals:signal.default_int_handlerrequires(signum, frame). The buggy line calls it with a single positional arg, catches the resultingTypeErrornowhere, and kills the receiving process with an unhandled exception instead of the intended clean shutdown.Traceback observed in the wild on Salt 3008.1 when SIGTERM was delivered to the master:
MasterPubServerChannel._publish_daemoncrashed andProcessManagerdid NOT respawn it.Why is the elif arm reachable?
signal.getsignal(SIGTERM)returnssignal.Handlers.SIG_DFL(an enum, not callable and not None) in a fresh interpreter, so the first two arms of theiffall through to the buggy one.Fix
Pass through the received
(signum, frame)tuple.default_int_handlerraisesKeyboardInterruptregardless of signum, matching the original intent.Test
tests/pytests/functional/utils/test_process.py::test_handle_signals_default_int_handler_typeerrorforces the child branch, sets_sigterm_handlertoSIG_DFL, and asserts_handle_signalsraisesKeyboardInterrupt(wasTypeErrorbefore this patch).Backporting: bug also present on
3007.x,3008.x,master; will forward-port via merge-forwards.