🛡️ Sentinel: [MEDIUM] Fix Process deadlocks by reading pipes before waitUntilExit#82
🛡️ Sentinel: [MEDIUM] Fix Process deadlocks by reading pipes before waitUntilExit#82NSEvent wants to merge 1 commit into
Conversation
…aitUntilExit Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughMultiple subprocess-handling code paths are reordered so that pipe output is fully read before calling waitUntilExit() or checking terminationStatus, addressing a potential deadlock when child process output exceeds pipe buffer capacity. A sentinel markdown file documents this vulnerability class. ChangesPipe Deadlock Fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift`:
- Around line 1408-1410: The stderr pipe in UniversalControlMouseRelay’s
tailscale command handling is never drained, so the deadlock risk remains even
though stdout is read before waitUntilExit(). Update the Process setup around
the tailscale invocation to stop using a Pipe for process.standardError and
redirect stderr to FileHandle.nullDevice instead, following the existing pattern
used in AutomationExecutor’s childPIDs helper. Keep the stdout read and
terminationStatus check as-is.
In
`@XboxControllerMapper/XboxControllerMapperTests/OBSWebSocketLiveIntegrationTests.swift`:
- Around line 170-172: In OBSWebSocketLiveIntegrationTests, the which process is
started with try? which.run() and then its stdout is read with
readDataToEndOfFile(), which can hang if the process never launches; switch this
flow to the same do-catch/early-return pattern used in AutomationExecutor so you
only read from outPipe after a successful run and process exit. Also address the
undrained which.standardError Pipe by redirecting stderr to a non-blocking sink
or null device so the child cannot deadlock if it writes heavily to stderr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7657df77-c615-4795-9234-4b2f6630865a
📒 Files selected for processing (4)
.Jules/sentinel.mdTriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swiftXboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swiftXboxControllerMapper/XboxControllerMapperTests/OBSWebSocketLiveIntegrationTests.swift
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return [] } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stderr pipe not drained — same deadlock risk remains for stderr.
The stdout read is correctly reordered before waitUntilExit(), but process.standardError (line 1400) is set to a Pipe() that is never read. If the tailscale command writes more than the OS pipe buffer (~64KB) to stderr, the child blocks on the write while the parent blocks on readDataToEndOfFile() or waitUntilExit() — the exact deadlock this PR aims to prevent.
Since stderr output is not used here, redirect it to FileHandle.nullDevice, matching the pattern already used in childPIDs in AutomationExecutor.swift.
🔧 Proposed fix (line 1400, outside selected range)
- process.standardError = Pipe()
+ process.standardError = FileHandle.nullDevice📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | |
| process.waitUntilExit() | |
| guard process.terminationStatus == 0 else { return [] } | |
| process.standardError = FileHandle.nullDevice |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift`
around lines 1408 - 1410, The stderr pipe in UniversalControlMouseRelay’s
tailscale command handling is never drained, so the deadlock risk remains even
though stdout is read before waitUntilExit(). Update the Process setup around
the tailscale invocation to stop using a Pipe for process.standardError and
redirect stderr to FileHandle.nullDevice instead, following the existing pattern
used in AutomationExecutor’s childPIDs helper. Keep the stdout read and
terminationStatus check as-is.
| let data = outPipe.fileHandleForReading.readDataToEndOfFile() | ||
| which.waitUntilExit() | ||
| if which.terminationStatus == 0 { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
try? which.run() can cause indefinite block on pipe read; stderr pipe also undrained.
Line 169 uses try? to discard run errors, then line 170 calls readDataToEndOfFile(). If which.run() fails (process never starts), the pipe's write end remains open and readDataToEndOfFile() blocks indefinitely. The previous code gated the pipe read behind terminationStatus == 0, which could skip the read on failure. The other call sites in AutomationExecutor use do-catch with early returns — this one should too.
Additionally, which.standardError (line 168) is set to an undrained Pipe(), leaving the same stderr deadlock risk if the child writes >64KB to stderr.
🔧 Proposed fixes (lines 168-169, outside selected range)
Handle the run error before reading:
- try? which.run()
+ do {
+ try which.run()
+ } catch {
+ throw XCTSkip("mediamtx not found. Install with `brew install mediamtx` or set MEDIAMTX_BIN")
+ }Redirect stderr to null device:
- which.standardError = Pipe()
+ which.standardError = FileHandle.nullDevice📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let data = outPipe.fileHandleForReading.readDataToEndOfFile() | |
| which.waitUntilExit() | |
| if which.terminationStatus == 0 { | |
| which.standardError = FileHandle.nullDevice | |
| do { | |
| try which.run() | |
| } catch { | |
| throw XCTSkip("mediamtx not found. Install with `brew install mediamtx` or set MEDIAMTX_BIN") | |
| } | |
| let data = outPipe.fileHandleForReading.readDataToEndOfFile() | |
| which.waitUntilExit() | |
| if which.terminationStatus == 0 { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@XboxControllerMapper/XboxControllerMapperTests/OBSWebSocketLiveIntegrationTests.swift`
around lines 170 - 172, In OBSWebSocketLiveIntegrationTests, the which process
is started with try? which.run() and then its stdout is read with
readDataToEndOfFile(), which can hang if the process never launches; switch this
flow to the same do-catch/early-return pattern used in AutomationExecutor so you
only read from outPipe after a successful run and process exit. Also address the
undrained which.standardError Pipe by redirecting stderr to a non-blocking sink
or null device so the child cannot deadlock if it writes heavily to stderr.
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: DoS / Resource Exhaustion via
Foundation.Processdeadlock. When a child process writes more data than the OS pipe buffer capacity (typically ~64KB), the child blocks on writing. If the parent application is simultaneously blocked onprocess.waitUntilExit()without draining the pipe, a thread deadlock occurs, leading to application freeze.🎯 Impact: An attacker or misbehaving automation step could cause the main application thread to hang indefinitely, resulting in a Denial of Service.
🔧 Fix: Refactored multiple occurrences across the codebase (
UniversalControlMouseRelay,AutomationExecutor,OBSWebSocketLiveIntegrationTests) to always read frompipe.fileHandleForReading.readDataToEndOfFile()before invokingprocess.waitUntilExit().✅ Verification: Verified via static code syntax checks and code review that the correct sequence is followed.
PR created automatically by Jules for task 3474175059985067787 started by @NSEvent
Summary by CodeRabbit