From ca76f01bb56d2457ccc7ffb5dcfab541e91da1c4 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Fri, 19 Sep 2025 04:28:28 +0000 Subject: [PATCH] Fix terminal mode interference when piping to less MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #22785 The issue was that Bun was saving and restoring terminal settings for all TTY file descriptors at process initialization and exit, even when it never modified them. This could interfere with programs like 'less' that need to control the terminal when receiving piped input. The fix follows libuv's approach: - Only save terminal settings when we actually modify them via Bun__ttySetMode - Don't preemptively save terminal settings at initialization - Only restore settings for file descriptors we actually modified This prevents Bun from interfering with terminal control when stdout is piped to pagers like 'less'. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- src/bun.js/bindings/c-bindings.cpp | 42 ++++----- src/bun.js/bindings/wtf-bindings.cpp | 38 +++++++++ .../issue/22785-less-terminal-mode.test.ts | 85 +++++++++++++++++++ 3 files changed, 144 insertions(+), 21 deletions(-) create mode 100644 test/regression/issue/22785-less-terminal-mode.test.ts diff --git a/src/bun.js/bindings/c-bindings.cpp b/src/bun.js/bindings/c-bindings.cpp index 4d91a42850..8d61988731 100644 --- a/src/bun.js/bindings/c-bindings.cpp +++ b/src/bun.js/bindings/c-bindings.cpp @@ -382,7 +382,9 @@ extern "C" ssize_t pwritev2(int fd, const struct iovec* iov, int iovcnt, extern "C" void Bun__onExit(); extern "C" int32_t bun_stdio_tty[3]; #if !OS(WINDOWS) -static termios termios_to_restore_later[3]; +termios termios_to_restore_later[3]; +// Track which terminal settings were actually saved +bool termios_was_saved[3] = { false, false, false }; #endif extern "C" void bun_restore_stdio() @@ -392,7 +394,7 @@ extern "C" void bun_restore_stdio() // restore stdio for (int32_t fd = 0; fd < 3; fd++) { - if (!bun_stdio_tty[fd]) + if (!bun_stdio_tty[fd] || !termios_was_saved[fd]) continue; sigset_t sa; @@ -459,7 +461,7 @@ extern "C" void bun_initialize_process() #if OS(LINUX) || OS(DARWIN) int devNullFd_ = -1; - bool anyTTYs = false; + // bool anyTTYs = false; // No longer needed since we don't set up signal handlers here const auto setDevNullFd = [&](int target_fd) -> void { bun_is_stdio_null[target_fd] = 1; @@ -494,15 +496,10 @@ extern "C" void bun_initialize_process() } } else { bun_stdio_tty[fd] = 1; - int err = 0; - do { - err = tcgetattr(fd, &termios_to_restore_later[fd]); - } while (err == -1 && errno == EINTR); - - if (err == 0) [[likely]] { - anyTTYs = true; - } + // Following libuv's approach: DO NOT save any terminal settings at initialization + // Only save them when/if we actually modify them via Bun__ttySetMode + // This prevents interfering with programs like 'less' that need terminal control } } @@ -511,18 +508,21 @@ extern "C" void bun_initialize_process() close(devNullFd_); } - // Restore TTY state on exit - if (anyTTYs) { - struct sigaction sa; - memset(&sa, 0, sizeof(sa)); - sigemptyset(&sa.sa_mask); + // Only restore TTY state on exit if we're actually modifying terminal settings + // Don't set up signal handlers just because stdout/stderr are TTYs, as this + // can interfere with programs like 'less' that control stdin when our output is piped + // Signal handlers will be set up by Bun__ttySetMode if/when terminal modes are actually changed + // if (anyTTYs) { + // struct sigaction sa; + // memset(&sa, 0, sizeof(sa)); + // sigemptyset(&sa.sa_mask); - sa.sa_flags = SA_RESETHAND; - sa.sa_handler = onExitSignal; + // sa.sa_flags = SA_RESETHAND; + // sa.sa_handler = onExitSignal; - sigaction(SIGTERM, &sa, nullptr); - sigaction(SIGINT, &sa, nullptr); - } + // sigaction(SIGTERM, &sa, nullptr); + // sigaction(SIGINT, &sa, nullptr); + // } #elif OS(WINDOWS) for (int fd = 0; fd <= 2; ++fd) { auto handle = reinterpret_cast(uv_get_osfhandle(fd)); diff --git a/src/bun.js/bindings/wtf-bindings.cpp b/src/bun.js/bindings/wtf-bindings.cpp index 7d9d6fb10d..01833b9a54 100644 --- a/src/bun.js/bindings/wtf-bindings.cpp +++ b/src/bun.js/bindings/wtf-bindings.cpp @@ -23,6 +23,15 @@ static std::once_flag reset_once_flag; static int current_tty_mode = 0; static struct termios orig_tty_termios; +// External declarations for terminal restoration +extern "C" int32_t bun_stdio_tty[3]; +extern struct termios termios_to_restore_later[3]; +extern bool termios_was_saved[3]; + +#include +extern "C" void onExitSignal(int sig); +static std::once_flag signal_handler_once_flag; + int uv__tcsetattr(int fd, int how, const struct termios* term) { int rc; @@ -124,6 +133,13 @@ extern "C" int Bun__ttySetMode(int fd, int mode) if (orig_termios_fd == -1) { orig_termios = orig_tty_termios; orig_termios_fd = fd; + + // Save the terminal settings for restoration later + // This follows libuv's approach - only save when we actually modify + if (fd >= 0 && fd <= 2 && bun_stdio_tty[fd]) { + termios_to_restore_later[fd] = orig_tty_termios; + termios_was_saved[fd] = true; + } } atomic_store(&orig_termios_spinlock, 0); @@ -146,6 +162,17 @@ extern "C" int Bun__ttySetMode(int fd, int mode) uv_tty_reset_mode(); }); }); + + // Set up signal handlers when we actually modify terminal settings + std::call_once(signal_handler_once_flag, [] { + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + sa.sa_handler = onExitSignal; + sigaction(SIGTERM, &sa, nullptr); + sigaction(SIGINT, &sa, nullptr); + }); break; case 2: // io uv__tty_make_raw(&tmp); @@ -155,6 +182,17 @@ extern "C" int Bun__ttySetMode(int fd, int mode) uv_tty_reset_mode(); }); }); + + // Set up signal handlers when we actually modify terminal settings + std::call_once(signal_handler_once_flag, [] { + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + sa.sa_handler = onExitSignal; + sigaction(SIGTERM, &sa, nullptr); + sigaction(SIGINT, &sa, nullptr); + }); break; } diff --git a/test/regression/issue/22785-less-terminal-mode.test.ts b/test/regression/issue/22785-less-terminal-mode.test.ts new file mode 100644 index 0000000000..6a20943bbc --- /dev/null +++ b/test/regression/issue/22785-less-terminal-mode.test.ts @@ -0,0 +1,85 @@ +import { test, expect } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import { mkdirSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +// Test for GitHub issue #22785 +// `bun script.js | less` sometimes causes terminal input to `less` to be line-buffered +test.skip("piping to less should not interfere with terminal mode", async () => { + // This test is skipped by default because: + // 1. It requires the 'less' command to be installed + // 2. It requires an interactive terminal + // 3. The issue is intermittent (~10% occurrence rate) + // + // To run this test manually: + // bun test test/regression/issue/22785-less-terminal-mode.test.ts + + const dir = tmpdir(); + const scriptPath = join(dir, "test-script.js"); + + // Create a script that outputs many lines + const scriptContent = ` + for (let i = 0; i < 1000; i++) { + process.stdout.write(\`\${i}\\n\`); + } + `; + writeFileSync(scriptPath, scriptContent); + + // Test multiple times since the issue is intermittent + const attempts = 50; + let failureCount = 0; + + for (let i = 0; i < attempts; i++) { + // Run bun piped to less and send 'q' to quit + // If the bug occurs, less will require Enter after 'q' + const proc = Bun.spawn({ + cmd: ["sh", "-c", `echo q | ${bunExe()} ${scriptPath} | less`], + env: { ...bunEnv, TERM: "xterm" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + ]); + + // If 'q' appears in stdout, it means less didn't quit immediately + // and displayed the 'q' character (bug occurred) + if (stdout.includes("q")) { + failureCount++; + } + + await proc.exited; + } + + // The bug should not occur with our fix + expect(failureCount).toBe(0); +}); + +// Test that stderr being redirected prevents the issue (as reported) +test.skip("piping to less with stderr redirected should work", async () => { + const dir = tmpdir(); + const scriptPath = join(dir, "test-script.js"); + + const scriptContent = ` + for (let i = 0; i < 100; i++) { + process.stdout.write(\`\${i}\\n\`); + } + `; + writeFileSync(scriptPath, scriptContent); + + // With stderr redirected, the issue should never occur + const proc = Bun.spawn({ + cmd: ["sh", "-c", `echo q | ${bunExe()} ${scriptPath} 2>/dev/null | less`], + env: { ...bunEnv, TERM: "xterm" }, + stdout: "pipe", + }); + + const stdout = await proc.stdout.text(); + await proc.exited; + + // 'q' should not appear in output + expect(stdout).not.toContain("q"); +}); \ No newline at end of file