Skip to content

🛡️ Sentinel: [MEDIUM] Fix Process deadlocks by reading pipes before waitUntilExit #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NSEvent wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [MEDIUM] Fix Process deadlocks by reading pipes before waitUntilExit #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .Jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,7 @@
**Vulnerability:** The application was falling back to storing OBS passwords as plaintext strings inside exported/saved JSON models (`Macro.swift` and `SystemCommand.swift`) if saving to the macOS Keychain failed.
**Learning:** Saving secrets to unencrypted formats simply because secure storage fails is a critical anti-pattern known as "failing open" that results in data exposure.
**Prevention:** Always fail securely. If secure storage operations fail, discard the sensitive credential in memory rather than writing it insecurely to disk, even if it requires the user to re-authenticate later.
## 2026-06-25 - [Process Output Pipe Deadlock (DoS)]
**Vulnerability:** Calling `process.waitUntilExit()` on a `Foundation.Process` while concurrently having the child process write to standard output or standard error without draining the associated `Pipe` can result in a thread deadlock. If the child process output exceeds the OS pipe buffer size (typically ~64KB), the child blocks on writing, and the parent blocks on waiting for the child to exit, freezing the application thread (Denial of Service).
**Learning:** System pipes have finite capacity. A parent process waiting on a child must actively drain the child's pipes to ensure the child can complete execution and exit.
**Prevention:** Always read from the `pipe.fileHandleForReading` (e.g., using `readDataToEndOfFile()`) *before* invoking `process.waitUntilExit()`, or use asynchronous pipe reading mechanisms (like `readabilityHandler`).
6 changes: 3 additions & 3 deletions TriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,9 +354,8 @@ public final class AutomationExecutor {
} catch {
return .failure("\(name) launch failed: \(error.localizedDescription)")
}
process.waitUntilExit()

let data = pipe.fileHandleForReading.readDataToEndOfFile()
process.waitUntilExit()
let output = String(data: data, encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""

Expand Down Expand Up @@ -729,13 +728,14 @@ private final class ShellCommandRunner: @unchecked Sendable {
let pipe = Pipe()
pgrep.standardOutput = pipe
pgrep.standardError = FileHandle.nullDevice
let data: Data
do {
try pgrep.run()
data = pipe.fileHandleForReading.readDataToEndOfFile()
pgrep.waitUntilExit()
} catch {
return []
}
let data = pipe.fileHandleForReading.readDataToEndOfFile()
let output = String(data: data, encoding: .utf8) ?? ""
return output
.split(whereSeparator: \.isNewline)
Expand Down
3 changes: 1 addition & 2 deletions XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,10 +1405,9 @@ final class UniversalControlMouseRelay: @unchecked Sendable {
NSLog("[UCMouseRelay] Could not run tailscale status: %@", String(describing: error))
return []
}
let data = pipe.fileHandleForReading.readDataToEndOfFile()
process.waitUntilExit()
guard process.terminationStatus == 0 else { return [] }
Comment on lines +1408 to 1410

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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 = pipe.fileHandleForReading.readDataToEndOfFile()
guard let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any],
let peers = object["Peer"] as? [String: Any] else {
return []
Expand Down
2 changes: 1 addition & 1 deletion XboxControllerMapper/XboxControllerMapperTests/OBSWebSocketLiveIntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,9 @@ private enum OBSMediaMTXManager {
which.standardOutput = outPipe
which.standardError = Pipe()
try? which.run()
let data = outPipe.fileHandleForReading.readDataToEndOfFile()
which.waitUntilExit()
if which.terminationStatus == 0 {
Comment on lines +170 to 172

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

let data = outPipe.fileHandleForReading.readDataToEndOfFile()
if let path = String(data: data, encoding: .utf8)?
.trimmingCharacters(in: .whitespacesAndNewlines),
!path.isEmpty,
Expand Down
Loading