Fix missing ICANON regression after read in noninteractive shell

Since commit 7e3fac561d (Query terminal only just before reading
from it, 2025-09-25),

	fish -c 'read; cat'

fails to turn ICANON back on for cat.

Even before that commit, I don't know how this ever worked.  Before or
after, we'd call tcsetattr() only to set our shell modes, not the
ones for external commands (term_donate)

I also don't think there's a good reason why we set modes twice
(between term_donate () and old_modes), but I'll make a minimal (=
safer) change for now until I understand this.

The only change from 7e3fac561d was that instead of doing things in
this order:

	terminal_init()
		set_shell_modes
	read
		reader_push()
		reader_readline()
			save & restore old_modes
	cat

we now do

	read
		reader_push()
			terminal_init()
				set_shell_modes()
		reader_readline()
			save & restore old_modes
	cat

So we call set_shell_modes() just before saving modes,
which obviously makes us save the wrong thing.
Again, not sure how that wasn't a problem before.

Fix it by saving modes before calling terminal_init().

Fixes #12024
This commit is contained in:
Johannes Altmanninger
2025-11-06 09:29:49 +01:00
parent 7aa64dc423
commit 60b50afefd
4 changed files with 61 additions and 24 deletions

View File

@@ -23,6 +23,7 @@ Interactive improvements
Scripting improvements
----------------------
- Fixed a regression from 4.1.0 where ``fish -c 'read; cat`` would set wrong terminal modes (:issue:`12024`).
Other improvements
------------------

View File

@@ -18,7 +18,7 @@
use crate::reader::ReaderConfig;
use crate::reader::commandline_set_buffer;
use crate::reader::reader_save_screen_state;
use crate::reader::{reader_pop, reader_push, reader_readline};
use crate::reader::{reader_pop, reader_push, reader_readline, set_shell_modes_temporarily};
use crate::tokenizer::TOK_ACCEPT_UNFINISHED;
use crate::tokenizer::TOK_ARGUMENT_LIST;
use crate::tokenizer::Tok;
@@ -276,6 +276,8 @@ fn read_interactive(
..Default::default()
};
let old_modes = set_shell_modes_temporarily(inputfd);
// Keep in-memory history only.
reader_push(parser, L!(""), conf);
let _modifiable_commandline = parser.scope().readonly_commandline.then(|| {
@@ -291,7 +293,7 @@ fn read_interactive(
let _interactive = parser.push_scope(|s| s.is_interactive = true);
let mut scoped_handoff = TtyHandoff::new(reader_save_screen_state);
scoped_handoff.enable_tty_protocols();
reader_readline(parser, nchars)
reader_readline(parser, old_modes, nchars)
};
if let Some(line) = mline {
*buff = line;

View File

@@ -814,7 +814,8 @@ fn read_i(parser: &Parser) {
while !check_exit_loop_maybe_warning(Some(&mut data)) {
RUN_COUNT.fetch_add(1, Ordering::Relaxed);
let Some(command) = data.readline(None) else {
let Some(command) = data.readline(set_shell_modes_temporarily(data.conf.inputfd), None)
else {
continue;
};
@@ -1165,10 +1166,14 @@ pub fn reader_reading_interrupted(data: &mut ReaderData) -> i32 {
/// characters even if a full line has not yet been read. Note: the returned value may be longer
/// than nchars if a single keypress resulted in multiple characters being inserted into the
/// commandline.
pub fn reader_readline(parser: &Parser, nchars: Option<NonZeroUsize>) -> Option<WString> {
pub fn reader_readline(
parser: &Parser,
old_modes: Option<libc::termios>,
nchars: Option<NonZeroUsize>,
) -> Option<WString> {
let data = current_data().unwrap();
let mut reader = Reader { parser, data };
reader.readline(nchars)
reader.readline(old_modes, nchars)
}
/// Get the command line state. This may be fetched on a background thread.
@@ -2283,7 +2288,11 @@ fn jump(
impl<'a> Reader<'a> {
/// Read a command to execute, respecting input bindings.
/// Return the command, or none if we were asked to cancel (e.g. SIGHUP).
fn readline(&mut self, nchars: Option<NonZeroUsize>) -> Option<WString> {
fn readline(
&mut self,
old_modes: Option<libc::termios>,
nchars: Option<NonZeroUsize>,
) -> Option<WString> {
let mut tty = TtyHandoff::new(reader_save_screen_state);
self.rls = Some(ReadlineLoopState::new());
@@ -2302,19 +2311,6 @@ fn readline(&mut self, nchars: Option<NonZeroUsize>) -> Option<WString> {
self.history_search.reset();
// It may happen that a command we ran when job control was disabled nevertheless stole the tty
// from us. In that case when we read from our fd, it will trigger SIGTTIN. So just
// unconditionally reclaim the tty. See #9181.
unsafe { libc::tcsetpgrp(self.conf.inputfd, libc::getpgrp()) };
// Get the current terminal modes. These will be restored when the function returns.
let mut old_modes = MaybeUninit::uninit();
let restore_modes =
unsafe { libc::tcgetattr(self.conf.inputfd, old_modes.as_mut_ptr()) } == 0;
// Set the new modes.
set_shell_modes(self.conf.inputfd, "readline");
// HACK: Don't abandon line for the first prompt, because
// if we're started with the terminal it might not have settled,
// so the width is quite likely to be in flight.
@@ -2389,11 +2385,13 @@ fn readline(&mut self, nchars: Option<NonZeroUsize>) -> Option<WString> {
if EXIT_STATE.load(Ordering::Relaxed) != ExitState::FinishedHandlers as _ {
// The order of the two conditions below is important. Try to restore the mode
// in all cases, but only complain if interactive.
if restore_modes
&& unsafe { libc::tcsetattr(self.conf.inputfd, TCSANOW, old_modes.as_ptr()) } == -1
&& is_interactive_session()
{
perror("tcsetattr");
// TODO(MSRV>=1.88) if-let-chain
if let Some(old_modes) = old_modes {
if unsafe { libc::tcsetattr(self.conf.inputfd, TCSANOW, &old_modes) } == -1
&& is_interactive_session()
{
perror("tcsetattr");
}
}
Outputter::stdoutput().borrow_mut().reset_text_face();
}
@@ -4457,6 +4455,25 @@ pub fn set_shell_modes(fd: RawFd, whence: &str) -> bool {
ok
}
pub fn set_shell_modes_temporarily(inputfd: RawFd) -> Option<libc::termios> {
// It may happen that a command we ran when job control was disabled nevertheless stole the tty
// from us. In that case when we read from our fd, it will trigger SIGTTIN. So just
// unconditionally reclaim the tty. See #9181.
unsafe { libc::tcsetpgrp(inputfd, libc::getpgrp()) };
// Get the current terminal modes. These will be restored when the function returns.
let old_modes = {
let mut old_modes = MaybeUninit::uninit();
let ok = unsafe { libc::tcgetattr(inputfd, old_modes.as_mut_ptr()) } == 0;
ok.then(|| unsafe { old_modes.assume_init() })
};
// Set the new modes.
set_shell_modes(inputfd, "readline");
old_modes
}
/// Grab control of terminal.
fn term_steal(copy_modes: bool) {
if copy_modes {

View File

@@ -17,3 +17,20 @@ tmux-sleep
string match <output -r '.*\e\]133;C.*' |
string escape
# CHECK: {{.*}}interactive{{$}}
isolated-tmux send-keys \
'$fish -c "read; cat"' Enter
tmux-sleep
isolated-tmux send-keys \
'read-value ' Enter
tmux-sleep
isolated-tmux send-keys cat1 Enter
tmux-sleep
isolated-tmux send-keys cat2 Enter
tmux-sleep
isolated-tmux capture-pane -p -S 2
# CHECK: read> read-value
# CHECK: read-value cat1
# CHECK: cat1
# CHECK: cat2
# CHECK: cat2