Background node process corrupts terminal state with tcsetattr() on exit

[andersk]

• Version: v14.9.0
• Platform: NixOS 21.03, Linux 5.8.11 x86_64
• Subsystem: ResetStdio in src/node.cc

In Bash, run the following:

node -e 'setTimeout(() => {}, 2000)' & sleep 1

This launches node -e 'setTimeout(() => {}, 2000)' in the background and sleep 1 in the foreground. After 1 second, control returns to Bash. After 2 seconds, Node exits and corrupts Bash’s terminal state:

• the ↑, ↓, →, ← arrow keys start printing ^[[A, ^[[B, ^[[C, ^[[D instead of scrolling through the Bash history or moving the insert point,
• the Tab key starts moving the cursor 8 spaces forward instead of Tab-completing the command,
• Ctrl+A and Ctrl+E print ^A and ^E instead of jumping to the beginning and end of the command, etc.

This happens when the ResetStdio handler uses tcsetattr to “restore” the terminal to the state it was in when Node started (#24260). This interferes with Bash, which uses and expects a different terminal state than sleep 1.

(The bug is sometimes reproducible without the sleep 1 depending on whether Node records the initial terminal state before Bash updates it, but the sleep 1 makes it reliable.)

Normally, the terminal state would be protected from such undesired modifications by a background process: the kernel generates SIGTTOU to suspend the process until it’s brought to the foreground. But ResetStdio now deliberately overrides this protection by blocking SIGTTOU (#28535).

IMO, both #24260 and #28535 should be reverted. Node is a programming language; it should never make changes to the terminal state that programs did not request.

[gireeshpunathil]

able to reproduce in linux, not sure what implications are for reverting the two commits in the linked PR. tag. Tagging as a bug.

[graue]

This also reproduces in Node 20.5.1 and 18.16.1 by:

1. Run any Node.js process in the foreground
2. Ctrl-Z it
3. Background it by entering bg
4. The process subsequently exits while backgrounded

I have a probably unusual use case where I hit this a lot and it's frustrating (I wrote a simple music player using Node that exits after playing the song). I concur that the behavior should be changed.

[bnoordhuis]

I don't think reverting #24260 or #28535 is realistically an option because that impacts a lot more users than this bug report. I personally don't care if node suspends on SIGTTOU or not, but enough people did to flag it as a bug.

My advice is to simply not background node but, if you absolutely have to, then as a workaround, reopen file descriptors 0-2 to /dev/null right before exiting. That will stop node from issuing the tcsetattr() calls.

Unless you or someone else has an actionable way forward, I'll go and close this issue in a few days.

[andersk]

Of course #28535 impacts many users given #24260. That’s why the proposal is to revert #24260 and #28535. If Node does not make unrequested changes to the terminal state on exit, then there will be no SIGTTOU and no suspension.

This issue affects more people than those who realize it, because it is so unexpected. It’s not just about a Node program running in the background; that’s just the most reliable way to reproduce. It also makes for a race condition that nondeterministically breaks piping a Node program to a pager, because the Node program exits while the pager is still supposed to be in charge of the terminal mode. See for example

• Keyboard arrow keys not working for scrolling from STDIN, rather shows keycode of the key pressed sharkdp/bat#1006
• Terminal needs reset if used with fzf sharkdp/bat#1650

Node programs that intentionally modify the terminal state are rare. It is the responsibility of those programs to restore the terminal state, and it’s reasonable to expect users to avoid running those programs in the background or piping them to a pager. But it’s not reasonable to say that typical programs that just console.log() things can’t safely be backgrounded or piped like programs in any other language.

[graue]

This issue affects more people than those who realize it, because it is so unexpected.

That seems true to me. As a case in point, I've been hitting this bug regularly for years, and it only just occurred to me to try to isolate the cause, figuring out it was a bug in Node and not my own program, and perform some lucky web searches that happened to lead me to this issue.

I skimmed #24260 and the issues it was trying to solve. I might be a little out of my depth here, but would it be reasonable to only make this tcsetattr() call on exit if stdin/stdout had previously been set to raw mode? Seems like it's trying to work around relatively specific cases where specific calls were made by the program.

[bnoordhuis]

That’s why the proposal is to revert #24260 and #28535.

Just to be clear: I'm saying reverting isn't an option. We're essentially at a local optimum where things work well enough for most users, and while I sympathize with your use case, anything that negatively impacts the silent majority is a no-go.

But it’s not reasonable to say that typical programs that just console.log() things can’t safely be backgrounded

With all due respect, you're betraying a lack of insight here in how node works under the hood. It goes to great lengths to provide an async runtime and that includes things like console.log.

would it be reasonable to only make this tcsetattr() call on exit if stdin/stdout had previously been set to raw mode?

I considered that when I wrote #24260 but it's unreliable with libraries like blessed (popular, ca. 10M downloads/month) because not all tty state changes are visible to node. The unconditional reset acts as a fail-safe that restores it to a sane state on program exit.

A command line flag to disable the current behavior might be an acceptable way forward (because that makes it opt-in) but to set expectations, I don't plan on working on or reviewing that myself, you'll have to find someone else.

[ritschwumm]

might the proposed reverts fix #42433 too? i currently have a very strange case where this occurs when node should just cleanly exit.