diff --git a/.Jules/sentinel.md b/.Jules/sentinel.md index 70f67594..16e9fbbf 100644 --- a/.Jules/sentinel.md +++ b/.Jules/sentinel.md @@ -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-28 - [Deadlock / DoS via Process() output buffer] +**Vulnerability:** Execution frameworks utilizing `Foundation.Process` combined with `Pipe()` were calling `process.waitUntilExit()` before reading the output `pipe.fileHandleForReading.readDataToEndOfFile()`. This allows a child process exceeding the OS pipe buffer size (~64KB) to block indefinitely while trying to write, creating a deadlock or DoS condition. +**Learning:** Process deadlocks can occur when output exceeds the pipe's internal buffer, because the operating system pauses the process until the buffer is read. Calling `waitUntilExit()` while the process is blocked on a write causes the parent process to hang forever. +**Prevention:** Always ensure you read pipe data (e.g., via `fileHandleForReading.readDataToEndOfFile()`) *before* invoking `process.waitUntilExit()`, or process reads asynchronously, to avoid deadlocking when capturing output. diff --git a/TriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swift b/TriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swift index 59854205..a44260bf 100644 --- a/TriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swift +++ b/TriggerKit/Sources/TriggerKitRuntime/AutomationExecutor.swift @@ -354,9 +354,11 @@ public final class AutomationExecutor { } catch { return .failure("\(name) launch failed: \(error.localizedDescription)") } - process.waitUntilExit() + // Read pipe data BEFORE waitUntilExit to avoid deadlock when + // output exceeds the pipe buffer size (~64KB). let data = pipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() let output = String(data: data, encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" @@ -641,7 +643,12 @@ private final class ShellCommandRunner: @unchecked Sendable { } let timeout = DispatchTime.now() + step.timeoutSeconds - if semaphore.wait(timeout: timeout) == .timedOut { + let waitResult = semaphore.wait(timeout: timeout) + + // If the process hasn't exited but pipe buffer is filled, it could be deadlocked + // but since we read asynchronously via readabilityHandler, this shouldn't happen for the pipe. + + if waitResult == .timedOut { terminateProcessTree(process, signal: SIGTERM) _ = semaphore.wait(timeout: .now() + 1) if process.isRunning { @@ -731,11 +738,13 @@ private final class ShellCommandRunner: @unchecked Sendable { pgrep.standardError = FileHandle.nullDevice do { try pgrep.run() - pgrep.waitUntilExit() } catch { return [] } + + // Read pipe data BEFORE waitUntilExit to avoid deadlock let data = pipe.fileHandleForReading.readDataToEndOfFile() + pgrep.waitUntilExit() let output = String(data: data, encoding: .utf8) ?? "" return output .split(whereSeparator: \.isNewline) diff --git a/TriggerKit/Tests/TriggerKitRuntimeTests/AutomationExecutorTests.swift b/TriggerKit/Tests/TriggerKitRuntimeTests/AutomationExecutorTests.swift index 4894b5b1..19bb79d3 100644 --- a/TriggerKit/Tests/TriggerKitRuntimeTests/AutomationExecutorTests.swift +++ b/TriggerKit/Tests/TriggerKitRuntimeTests/AutomationExecutorTests.swift @@ -464,7 +464,7 @@ final class AutomationExecutorTests: XCTestCase { ])) } - try? await Task.sleep(nanoseconds: 20_000_000) + try? await Task.sleep(nanoseconds: 250_000_000) // increase delay so process has time to start task.cancel() let result = await task.value