Skip to content

src: restore tty state on exit only if changed - #66753

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-stdio-termios-restore
Open

mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-stdio-termios-restore

Conversation

@mcollina

@mcollina mcollina commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

ResetStdio() unconditionally writes the terminal attributes recorded at startup back to stdin and stderr when the process exits, even when the process never touched the terminal. This clobbers changes made in the meantime by another process sharing the terminal. In node -h | less, less switches the terminal to raw mode once it receives the first byte of output, and node switches it back to cooked mode when it exits, so less ends up running with the terminal in cooked mode (see gwsw/less#834). The same mechanism is behind #35536 (backgrounded node process corrupting the shell's terminal state).

With this change the recorded attributes are only written back when node itself changed the terminal mode through setRawMode(), or when exiting because of a signal (SIGINT/SIGTERM, trap handler, watchdog timeout). The fail-safe from #24260 is therefore preserved for every program that asked node to change the terminal, including the setRawMode(true); process.stdin.destroy() case where libuv alone cannot reset the mode. Restoring the O_NONBLOCK flag and libuv's own uv_tty_reset_mode() are unchanged.

Verified with strace under a pty: node -e 'console.log(1)' | cat issues no tcsetattr() on exit anymore, and a consumer that enters raw mode after the first byte of output (like less 710) keeps raw mode after node exits. All test/pseudo-tty tests pass. The new test fails on the unpatched binary.

The behaviour change is for code that alters the terminal behind libuv's back (native addons, stty child processes with inherited stdio) and then exits normally without cleaning up: node no longer papers over that. Marking as semver-major for that reason.

Fixes: #66440
Refs: #35536
Refs: #66540


AI generated, reviewed by me

ResetStdio() unconditionally writes the terminal attributes recorded at
startup back to stdin and stderr when the process exits, even when the
process never touched the terminal. This clobbers changes made in the
meantime by another process sharing the terminal: in `node -h | less`,
less switches the terminal to raw mode once it receives the first byte
of output, and node switches it back to cooked mode when it exits.

Only restore the recorded attributes when node itself changed the
terminal mode through setRawMode(), or when exiting because of a signal.
Restoring the O_NONBLOCK flag and libuv's own terminal mode reset are
unchanged.

Fixes: nodejs#66440
Refs: nodejs#35536
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. tty Issues and PRs related to the tty subsystem. needs-ci PRs that need a full CI run. labels Oct 15, 2026
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 15, 2026
@codecov

codecov Bot commented Oct 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.49%. Comparing base (29bd740) to head (c61847d).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/node.cc 72.72% 2 Missing and 1 partial ⚠️
src/tty_wrap.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66753      +/-   ##
==========================================
+ Coverage   90.47%   90.49%   +0.02%     
==========================================
  Files         791      797       +6     
  Lines      277245   277501     +256     
  Branches    53310    53324      +14     
==========================================
+ Hits       250834   251126     +292     
+ Misses      16811    16796      -15     
+ Partials     9600     9579      -21     
Files with missing lines Coverage Δ
src/node_internals.h 80.35% <ø> (ø)
src/node_watchdog.cc 79.31% <ø> (ø)
src/tty_wrap.cc 81.92% <0.00%> (-1.00%) ⬇️
src/node.cc 79.60% <72.72%> (+0.35%) ⬆️

... and 53 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. tty Issues and PRs related to the tty subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Problem with newer less

2 participants