From 3e24e96af51fe308705a1f1695e3b9045c54482e Mon Sep 17 00:00:00 2001
From: Danny Lin <danny@kdrag0n.dev>
Date: Tue, 31 Dec 2024 02:01:46 -0800
Subject: [PATCH] termio/exec: fix SIGPIPE crash when reader exits early

If the read thread has already exited, it will have closed the read end
of the quit pipe. Unless SIGPIPE is masked with signal(SIGPIPE, SIG_IGN),
or the macOS-specific fcntl(F_SETNOSIGPIPE), writing to the write end of
a broken pipe kills the writer with SIGPIPE instead of returning -EPIPE
as an error. This causes a crash if the read thread exits before
threadExit.

This was already a possible race condition if read() returns
error.NotOpenForReading or error.InputOutput, but it's now much easier to
trigger due to the recent "termio/exec: fix 100% CPU usage after
wait-after-command process exits" fix.

Fix this by closing the quit pipe instead of writing to it.
---
 src/termio/Exec.zig | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig
index 1a3b8cad0..d409ccbb0 100644
--- a/src/termio/Exec.zig
+++ b/src/termio/Exec.zig
@@ -179,8 +179,11 @@ pub fn threadExit(self: *Exec, td: *termio.Termio.ThreadData) void {
     // Quit our read thread after exiting the subprocess so that
     // we don't get stuck waiting for data to stop flowing if it is
     // a particularly noisy process.
-    _ = posix.write(exec.read_thread_pipe, "x") catch |err|
-        log.warn("error writing to read thread quit pipe err={}", .{err});
+    if (exec.read_thread_pipe) |pipe| {
+        posix.close(pipe);
+        // Tell deinit that we've already closed the pipe
+        exec.read_thread_pipe = null;
+    }
 
     if (comptime builtin.os.tag == .windows) {
         // Interrupt the blocking read so the thread can see the quit message
@@ -639,7 +642,7 @@ pub const ThreadData = struct {
 
     /// Reader thread state
     read_thread: std.Thread,
-    read_thread_pipe: posix.fd_t,
+    read_thread_pipe: ?posix.fd_t,
     read_thread_fd: posix.fd_t,
 
     /// The timer to detect termios state changes.
@@ -652,7 +655,8 @@ pub const ThreadData = struct {
     termios_mode: ptypkg.Mode = .{},
 
     pub fn deinit(self: *ThreadData, alloc: Allocator) void {
-        posix.close(self.read_thread_pipe);
+        // If the pipe isn't closed, close it.
+        if (self.read_thread_pipe) |pipe| posix.close(pipe);
 
         // Clear our write pools. We know we aren't ever going to do
         // any more IO since we stop our data stream below so we can just
@@ -1433,9 +1437,12 @@ pub const ReadThread = struct {
                 };
 
                 // This happens on macOS instead of WouldBlock when the
-                // child process dies. To be safe, we just break the loop
-                // and let our poll happen.
-                if (n == 0) break;
+                // child process dies. It's equivalent to NotOpenForReading
+                // so we can just exit.
+                if (n == 0) {
+                    log.info("io reader exiting", .{});
+                    return;
+                }
 
                 // log.info("DATA: {d}", .{n});
                 @call(.always_inline, termio.Termio.processOutput, .{ io, buf[0..n] });
@@ -1447,8 +1454,8 @@ pub const ReadThread = struct {
                 return;
             };
 
-            // If our quit fd is set, we're done.
-            if (pollfds[1].revents & posix.POLL.IN != 0) {
+            // If our quit fd is closed, we're done.
+            if (pollfds[1].revents & posix.POLL.HUP != 0) {
                 log.info("read thread got quit signal", .{});
                 return;
             }