From 60b50afefd9de7b6a82916b931043af4d194f712 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Thu, 6 Nov 2025 09:29:49 +0100 Subject: [PATCH] Fix missing ICANON regression after read in noninteractive shell Since commit 7e3fac561d6 (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 7e3fac561d6 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 --- CHANGELOG.rst | 1 + src/builtins/read.rs | 6 ++- src/reader/reader.rs | 61 ++++++++++++++++++++----------- tests/checks/tmux-invocation.fish | 17 +++++++++ 4 files changed, 61 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d1957f658..4c8c7059a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -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 ------------------ diff --git a/src/builtins/read.rs b/src/builtins/read.rs index 5318d00e1..437917e1c 100644 --- a/src/builtins/read.rs +++ b/src/builtins/read.rs @@ -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; diff --git a/src/reader/reader.rs b/src/reader/reader.rs index d942c5b99..5acb07b64 100644 --- a/src/reader/reader.rs +++ b/src/reader/reader.rs @@ -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) -> Option { +pub fn reader_readline( + parser: &Parser, + old_modes: Option, + nchars: Option, +) -> Option { 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) -> Option { + fn readline( + &mut self, + old_modes: Option, + nchars: Option, + ) -> Option { let mut tty = TtyHandoff::new(reader_save_screen_state); self.rls = Some(ReadlineLoopState::new()); @@ -2302,19 +2311,6 @@ fn readline(&mut self, nchars: Option) -> Option { 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) -> Option { 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 { + // 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 { diff --git a/tests/checks/tmux-invocation.fish b/tests/checks/tmux-invocation.fish index 0485bf2e0..4b4c34345 100644 --- a/tests/checks/tmux-invocation.fish +++ b/tests/checks/tmux-invocation.fish @@ -17,3 +17,20 @@ tmux-sleep string match read-value +# CHECK: read-value cat1 +# CHECK: cat1 +# CHECK: cat2 +# CHECK: cat2