Skip to content

Commit

Permalink
termio/exec: fix SIGPIPE crash when reader exits early
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kdrag0n authored and mitchellh committed Jan 8, 2025
1 parent 542c655 commit 3e24e96
Showing 1 changed file with 16 additions and 9 deletions.
25 changes: 16 additions & 9 deletions src/termio/Exec.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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] });
Expand All @@ -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;
}
Expand Down

0 comments on commit 3e24e96

Please sign in to comment.