Repository navigation
Start every spawned child from a clean signal state - #15813
Open
lawrencecchen wants to merge 4 commits into
Open
lawrencecchen wants to merge 4 commits into
lawrencecchen wants to merge 4 commits into
Conversation
SimulatorPOSIXProcessLauncher, launched from a thread with blocked signals, hands the child that mask and an ignored disposition. Fails on this commit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A posix_spawn child inherits the calling thread's signal mask and every SIG_IGN disposition. cmux spawns from Swift concurrency and dispatch worker threads, which block nearly every signal, so such children never received SIGTERM or SIGWINCH. Add POSIXSpawnSignalPolicy to CmuxFoundation. It sets an empty mask and the default disposition for every catchable signal on a posix_spawnattr_t, keeping the flags already set. Apply it at all 18 posix_spawn sites in the app, its packages and the CLI, replacing the partial per-site resets. The upload command and notification hooks keep the app's ignored SIGPIPE, as before. CmuxSudoBroker and CMUXAgentLaunch now depend on CmuxFoundation, the leaf package every target already links. The CLI source guard now requires the policy at every posix_spawn site under CLI, Sources and Packages. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (23)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (23)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds a shared POSIX spawn signal policy that clears child signal masks and resets selected catchable signals to default dispositions. CLI, app, and package launch paths use the policy, with some callers preserving inherited dispositions. ChangesSpawn signal policy
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant SimulatorPOSIXProcessLauncher
participant POSIXSpawnSignalPolicy
participant posix_spawn
participant PerlProbe
SimulatorPOSIXProcessLauncher->>POSIXSpawnSignalPolicy: apply(to: attributes)
SimulatorPOSIXProcessLauncher->>posix_spawn: launch with configured attributes
posix_spawn->>PerlProbe: start process with signal policy
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Spawned processes receive clean signal state while upload and notification commands retain their intended SIGPIPE behavior. No actionable issue remains; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The shared policy makes child signal handling predictable while preserving inspected process-isolation controls. No introduced privilege expansion was established. Risk remains low rather than minimal because interruption behavior in authentication setup and some downstream launch paths was not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Revert the extension of the source scan to Sources/ and Packages/. The CLI check now accepts POSIXSpawnSignalPolicy, because the three CLI spawn sites use it instead of setting the mask by hand. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
Contributor
Dogfood tours of
|
This branch has not been deployed
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
2 participants
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.
Summary
A child that cmux starts with
posix_spawninherits the calling thread's signal mask and every ignored disposition. cmux spawns from Swift concurrency and dispatch worker threads, which block nearly every signal, so these children never received SIGTERM, SIGINT or SIGWINCH and could not shut down or resize. The acpmux daemon ignoring SIGTERM (#15521) was one case of this.POSIXSpawnSignalPolicy(CmuxFoundation) configures aposix_spawnattr_twith an empty mask and the default disposition for every catchable signal, addingPOSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEFto the flags already set. All 18posix_spawnsites in the app, its packages and the CLI now use it:RosettaNativeRelaunchAgentForkCommandOutputRunnerNotificationSoundSettings+CustomSoundStaging(2 calls)AutomationProcessSessionAgentRestoreLaunchLease(CMUXAgentLaunch)WorkspaceChangesGitProcess(CmuxGit)SimulatorPOSIXProcessLauncher(CmuxSimulator)SudoPOSIXProcessSpawner,SudoPrivilegedProcessSupervisor,SudoRunnerLauncher,SystemSudoAppLauncher,SystemSudoTouchIDSetupLauncherTerminalCustomUploadRunner,TerminalNotificationPolicyCodexTeamsAppServerProcessOwnedProcessSupervisorrestoreprovider preflightThe upload command and notification hooks keep the app's ignored SIGPIPE on purpose (a closed pipe returns
EPIPEinstead of killing them mid-cleanup, #12383), throughPOSIXSpawnSignalPolicy(inheritingDispositionsOf: [SIGPIPE]).Foundation
Processneeds no change. A probe spawned byProcessfrom a thread with every signal blocked and SIGINT, SIGQUIT, SIGHUP and SIGPIPE ignored reported an empty mask and default dispositions; the same probe through a bareposix_spawnreported the full mask and all four ignores. The CLI'sexecvesites already reset state throughcliExecFailureErrno. A non-interactive/bin/shstill starts&jobs with SIGINT and SIGQUIT ignored whatever the spawn attributes say; the only such launches are the CLI's fire-and-forget hook workers, which is harmless for them and left alone. Ghostty's terminal spawn (Pty.childPreExec) resets dispositions but not the mask; it forks from Ghostty's IO thread and is out of scope here.CmuxSudoBroker and CMUXAgentLaunch now depend on CmuxFoundation, the leaf package that every target linking them already links.
After #15521 merges,
AcpmuxDaemonLauncher.spawnDetachedshould switch to this policy; that PR keeps its own copy because it is not merged.Testing
SimulatorPOSIXProcessLauncherSignalStateTestslaunches a Perl probe through the real launcher from a thread with every signal blocked and SIGXFSZ ignored.swift test --filter SimulatorPOSIXProcessLauncherSignalStateTestsinPackages/macOS/CmuxSimulator: red on 45190ff (blocked=1 2 … 31 ignored=25), green on 5919e97.POSIXSpawnSignalPolicyTests(CmuxFoundation): the policy yields a clean child, the inherited-disposition option keeps only that signal, prior flags survive, and a control spawn without the policy shows the leak. 5 tests pass locally withswift test. Locally I had to drop the unrelatedSSHPTYReplayOutputFilterTests.swift, which Swift 6.4 cannot type-check in time; CI's toolchain is unaffected.swift testpasses for CMUXAgentLaunch and CmuxSudoBroker; CmuxGit builds.CLIExecInheritedSignalStateTestskeeps itsmainscope (CLI/only). Its spawn check now also acceptsPOSIXSpawnSignalPolicy, because the three CLI spawn sites use it; without that change the existing check fails.Changelog
Fixed: Helper processes cmux starts, such as sudo commands, git, simulator tools, upload commands and notification hooks, now receive SIGTERM and other signals instead of inheriting a blocked signal mask
🤖 Generated with Claude Code
Summary by CodeRabbit
SIGPIPEfor terminal notification hooks.