From debaa2cc34a0aaaf3ed73a45e2d11bda0de07ef7 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 6 Sep 2024 13:54:01 -0700 Subject: [PATCH] Fix CTRL + C behavior in `bun run` so it doesn't `^[[A` (#13762) --- src/bun.js/api/bun/process.zig | 39 ++++++++- src/bun.js/bindings/c-bindings.cpp | 113 ++++++++++++++++++++++++++- src/crash_handler.zig | 16 ++-- test/regression/issue/ctrl-c.test.ts | 35 +++++++++ 4 files changed, 194 insertions(+), 9 deletions(-) create mode 100644 test/regression/issue/ctrl-c.test.ts diff --git a/src/bun.js/api/bun/process.zig b/src/bun.js/api/bun/process.zig index 69a806df533869..198289a92741f4 100644 --- a/src/bun.js/api/bun/process.zig +++ b/src/bun.js/api/bun/process.zig @@ -2045,21 +2045,54 @@ pub const sync = struct { return spawnWithArgv(options, @ptrCast(args.items.ptr), @ptrCast(envp)); } + // Forward signals from parent to the child process. + extern "C" fn Bun__registerSignalsForForwarding() void; + extern "C" fn Bun__unregisterSignalsForForwarding() void; + + // The PID to forward signals to. + // Set to 0 when unregistering. + extern "C" var Bun__currentSyncPID: i64; + + // Race condition: a signal could be sent before spawnProcessPosix returns. + // We need to make sure to send it after the process is spawned. + extern "C" fn Bun__sendPendingSignalIfNecessary() void; + fn spawnPosix( options: *const Options, argv: [*:null]?[*:0]const u8, envp: [*:null]?[*:0]const u8, ) !Maybe(Result) { + Bun__currentSyncPID = 0; + Bun__registerSignalsForForwarding(); + defer { + Bun__unregisterSignalsForForwarding(); + bun.crash_handler.resetOnPosix(); + } const process = switch (try spawnProcessPosix(&options.toSpawnOptions(), argv, envp)) { .err => |err| return .{ .err = err }, .result => |proces| proces, }; + Bun__currentSyncPID = @intCast(process.pid); + + Bun__sendPendingSignalIfNecessary(); + var out = [2]std.ArrayList(u8){ std.ArrayList(u8).init(bun.default_allocator), std.ArrayList(u8).init(bun.default_allocator), }; var out_fds = [2]bun.FileDescriptor{ process.stdout orelse bun.invalid_fd, process.stderr orelse bun.invalid_fd }; + var success = false; defer { + // If we're going to return an error, + // let's make sure to clean up the output buffers + // and kill the process + if (!success) { + for (&out) |*array_list| { + array_list.clearAndFree(); + } + _ = std.c.kill(process.pid, 1); + } + for (out_fds) |fd| { if (fd != bun.invalid_fd) { _ = bun.sys.close(fd); @@ -2090,13 +2123,14 @@ pub const sync = struct { for (&out_fds_to_wait_for, &out, &out_fds) |*fd, *bytes, *out_fd| { if (fd.* == bun.invalid_fd) continue; while (true) { - bytes.ensureUnusedCapacity(16384) catch bun.outOfMemory(); + bytes.ensureUnusedCapacity(16384) catch { + return .{ .err = bun.sys.Error.fromCode(.NOMEM, .recv) }; + }; switch (bun.sys.recvNonBlock(fd.*, bytes.unusedCapacitySlice())) { .err => |err| { if (err.isRetry() or err.getErrno() == .PIPE) { break; } - _ = std.c.kill(process.pid, 1); return .{ .err = err }; }, .result => |bytes_read| { @@ -2165,6 +2199,7 @@ pub const sync = struct { } } + success = true; return .{ .result = Result{ .status = status, diff --git a/src/bun.js/bindings/c-bindings.cpp b/src/bun.js/bindings/c-bindings.cpp index 3c23788a4149cf..52542f03b5966d 100644 --- a/src/bun.js/bindings/c-bindings.cpp +++ b/src/bun.js/bindings/c-bindings.cpp @@ -623,4 +623,115 @@ extern "C" void Bun__disableSOLinger(SOCKET fd) setsockopt(fd, SOL_SOCKET, SO_LINGER, (char*)&l, sizeof(l)); } -#endif \ No newline at end of file +#endif + +// Handle signals in bun.spawnSync. +// If we receive a signal, we want to forward the signal to the child process. +#if OS(LINUX) || OS(DARWIN) +#include +#include + +// Note: We only ever use bun.spawnSync on the main thread. +extern "C" int64_t Bun__currentSyncPID = 0; +static int Bun__pendingSignalToSend = 0; +static struct sigaction previous_actions[NSIG]; + +// This list of signals is copied from npm. +// https://github.com/npm/cli/blob/fefd509992a05c2dfddbe7bc46931c42f1da69d7/workspaces/arborist/lib/signals.js#L26-L57 +#define FOR_EACH_POSIX_SIGNAL(M) \ + M(SIGABRT); \ + M(SIGALRM); \ + M(SIGHUP); \ + M(SIGINT); \ + M(SIGTERM); \ + M(SIGVTALRM); \ + M(SIGXCPU); \ + M(SIGXFSZ); \ + M(SIGUSR2); \ + M(SIGTRAP); \ + M(SIGSYS); \ + M(SIGQUIT); \ + M(SIGIOT); \ + M(SIGIO); + +#if OS(LINUX) +#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \ + M(SIGPOLL); \ + M(SIGPWR); \ + M(SIGSTKFLT); + +#endif + +#if OS(DARWIN) +#define FOR_EACH_SIGNAL(M) FOR_EACH_POSIX_SIGNAL(M) +#endif + +#if OS(LINUX) +#define FOR_EACH_SIGNAL(M) \ + FOR_EACH_POSIX_SIGNAL(M) \ + FOR_EACH_LINUX_ONLY_SIGNAL(M) +#endif + +static void Bun__forwardSignalFromParentToChildAndRestorePreviousAction(pid_t pid, int sig) +{ + sigset_t restore_mask; + sigset_t mask; + sigemptyset(&mask); + sigaddset(&mask, sig); + sigemptyset(&restore_mask); + sigaddset(&restore_mask, sig); + pthread_sigmask(SIG_BLOCK, &mask, &restore_mask); + kill(pid, sig); + pthread_sigmask(SIG_UNBLOCK, &restore_mask, nullptr); +} + +extern "C" void Bun__sendPendingSignalIfNecessary() +{ + int sig = Bun__pendingSignalToSend; + Bun__pendingSignalToSend = 0; + int pid = Bun__currentSyncPID; + if (sig == 0 || pid == 0) + return; + + Bun__forwardSignalFromParentToChildAndRestorePreviousAction(pid, sig); +} + +extern "C" void Bun__registerSignalsForForwarding() +{ + Bun__pendingSignalToSend = 0; + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + sa.sa_handler = [](int sig) { + if (Bun__currentSyncPID == 0) { + Bun__pendingSignalToSend = sig; + return; + } + + Bun__forwardSignalFromParentToChildAndRestorePreviousAction(Bun__currentSyncPID, sig); + }; + +#define REGISTER_SIGNAL(SIG) \ + if (sigaction(SIG, &sa, &previous_actions[SIG]) == -1) { \ + } + + FOR_EACH_SIGNAL(REGISTER_SIGNAL) + +#undef REGISTER_SIGNAL +} + +extern "C" void Bun__unregisterSignalsForForwarding() +{ + Bun__currentSyncPID = 0; + +#define UNREGISTER_SIGNAL(SIG) \ + if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \ + } + + FOR_EACH_SIGNAL(UNREGISTER_SIGNAL) + memset(previous_actions, 0, sizeof(previous_actions)); +#undef UNREGISTER_SIGNAL +} + +#endif diff --git a/src/crash_handler.zig b/src/crash_handler.zig index 811d86d456ecef..52408482d92d03 100644 --- a/src/crash_handler.zig +++ b/src/crash_handler.zig @@ -763,6 +763,15 @@ pub fn updatePosixSegfaultHandler(act: ?*std.posix.Sigaction) !void { var windows_segfault_handle: ?windows.HANDLE = null; +pub fn resetOnPosix() void { + var act = std.posix.Sigaction{ + .handler = .{ .sigaction = handleSegfaultPosix }, + .mask = std.posix.empty_sigset, + .flags = (std.posix.SA.SIGINFO | std.posix.SA.RESTART | std.posix.SA.RESETHAND), + }; + updatePosixSegfaultHandler(&act) catch {}; +} + pub fn init() void { if (!enable) return; switch (bun.Environment.os) { @@ -770,12 +779,7 @@ pub fn init() void { windows_segfault_handle = windows.kernel32.AddVectoredExceptionHandler(0, handleSegfaultWindows); }, .mac, .linux => { - var act = std.posix.Sigaction{ - .handler = .{ .sigaction = handleSegfaultPosix }, - .mask = std.posix.empty_sigset, - .flags = (std.posix.SA.SIGINFO | std.posix.SA.RESTART | std.posix.SA.RESETHAND), - }; - updatePosixSegfaultHandler(&act) catch {}; + resetOnPosix(); }, else => @compileError("TODO"), } diff --git a/test/regression/issue/ctrl-c.test.ts b/test/regression/issue/ctrl-c.test.ts new file mode 100644 index 00000000000000..57d0aa2c2141ca --- /dev/null +++ b/test/regression/issue/ctrl-c.test.ts @@ -0,0 +1,35 @@ +import { test, expect, it } from "bun:test"; +import { join } from "path"; +import { bunEnv, bunExe, isWindows, tempDirWithFiles } from "harness"; + +test.skipIf(isWindows)("verify that we forward SIGINT from parent to child in bun run", () => { + const dir = tempDirWithFiles("ctrlc", { + "index.js": ` + let count = 0; + process.exitCode = 1; + process.once("SIGINT", () => { + process.kill(process.pid, "SIGKILL"); + }); + setTimeout(() => {}, 999999) + process.kill(process.ppid, "SIGINT"); + `, + "package.json": ` + { + "name": "ctrlc", + "scripts": { + "start": "${bunExe()} index.js" + } + } + `, + }); + + const result = Bun.spawnSync({ + cmd: [bunExe(), "start"], + cwd: dir, + env: bunEnv, + stdout: "inherit", + stderr: "inherit", + }); + expect(result.exitCode).toBe(null); + expect(result.signalCode).toBe("SIGKILL"); +});