From 5c94ebd0954c2af5772adfc1818938958adbd7dc Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Wed, 28 Feb 2024 13:06:45 -0600 Subject: [PATCH] Fix output::stdoutput() safety issues Fairly straightforward, with the only unfortunate part of this being that `Screen` isn't as pure and now encodes the facte that we use it with main-thread-only stdout `Outputter`. --- src/output.rs | 14 +++++--------- src/reader.rs | 28 ++++++++++++++-------------- src/screen.rs | 48 ++++++++++++++++++++++++++---------------------- 3 files changed, 45 insertions(+), 45 deletions(-) diff --git a/src/output.rs b/src/output.rs index 33d9fcf78..c93aed11c 100644 --- a/src/output.rs +++ b/src/output.rs @@ -3,9 +3,9 @@ use crate::color::RgbColor; use crate::common::{self, wcs2string_appending}; use crate::curses::{self, tparm1, Term}; use crate::env::EnvVar; +use crate::threads::MainThread; use crate::wchar::prelude::*; use bitflags::bitflags; -use std::cell::RefCell; use std::ffi::CStr; use std::io::{Result, Write}; use std::os::fd::RawFd; @@ -444,14 +444,10 @@ impl Outputter { /// Access the outputter for stdout. /// This should only be used from the main thread. - pub fn stdoutput() -> &'static mut RefCell { - crate::threads::assert_is_main_thread(); - static mut STDOUTPUT: RefCell = - RefCell::new(Outputter::new_from_fd(libc::STDOUT_FILENO)); - // Safety: this is only called from the main thread. - // XXX: creating and using multiple (read or write!) references to the same mutable static - // is undefined behavior! - unsafe { &mut STDOUTPUT } + pub fn stdoutput() -> &'static MainThread { + static STDOUTPUT: MainThread = + MainThread::new(Outputter::new_from_fd(libc::STDOUT_FILENO)); + &STDOUTPUT } } diff --git a/src/reader.rs b/src/reader.rs index 43f0f0b6c..dd7089ed0 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -1902,8 +1902,7 @@ impl ReaderData { perror("tcsetattr"); // return to previous mode } Outputter::stdoutput() - .get_mut() - .set_color(RgbColor::RESET, RgbColor::RESET); + .with_mut(|output| output.set_color(RgbColor::RESET, RgbColor::RESET)); } rls.finished.then(|| zelf.command_line.text().to_owned()) } @@ -2942,8 +2941,9 @@ impl ReaderData { el.end_edit_group(); } rl::DisableMouseTracking => { - let outp = Outputter::stdoutput().get_mut(); - outp.write_wstr(L!("\x1B[?1000l")); + Outputter::stdoutput().with_mut(|outp| { + outp.write_wstr(L!("\x1B[?1000l")); + }); } rl::ClearScreenAndRepaint => { self.parser().libdata_mut().pods.is_repaint = true; @@ -2954,8 +2954,9 @@ impl ReaderData { // and *then* reexecute the prompt and overdraw it. // This removes the flicker, // while keeping the prompt up-to-date. - let outp = Outputter::stdoutput().get_mut(); - outp.write_wstr(&clear); + Outputter::stdoutput().with_mut(|outp| { + outp.write_wstr(&clear); + }); self.screen.reset_line(/*repaint_prompt=*/ true); self.layout_and_repaint(L!("readline")); } @@ -3487,9 +3488,9 @@ fn reader_interactive_init(parser: &Parser) { /// Destroy data for interactive use. fn reader_interactive_destroy() { - Outputter::stdoutput() - .get_mut() - .set_color(RgbColor::RESET, RgbColor::RESET); + Outputter::stdoutput().with_mut(|outp| { + outp.set_color(RgbColor::RESET, RgbColor::RESET); + }); } /// \return whether fish is currently unwinding the stack in preparation to exit. @@ -3570,9 +3571,9 @@ pub fn reader_write_title( let _ = write_loop(&STDOUT_FILENO, &narrow); } - Outputter::stdoutput() - .get_mut() - .set_color(RgbColor::RESET, RgbColor::RESET); + Outputter::stdoutput().with_mut(|outp| { + outp.set_color(RgbColor::RESET, RgbColor::RESET); + }); if reset_cursor_position && !lst.is_empty() { // Put the cursor back at the beginning of the line (issue #2453). let _ = write_to_fd(b"\r", STDOUT_FILENO); @@ -4583,9 +4584,8 @@ fn reader_run_command(parser: &Parser, cmd: &wstr) -> EvalRes { .set_one(L!("_"), EnvMode::GLOBAL, ft.to_owned()); } - let outp = Outputter::stdoutput().get_mut(); reader_write_title(cmd, parser, true); - outp.set_color(RgbColor::NORMAL, RgbColor::NORMAL); + Outputter::stdoutput().with_mut(|outp| outp.set_color(RgbColor::NORMAL, RgbColor::NORMAL)); term_donate(false); let time_before = Instant::now(); diff --git a/src/screen.rs b/src/screen.rs index 4ed5b51f0..648f48eb4 100644 --- a/src/screen.rs +++ b/src/screen.rs @@ -8,6 +8,7 @@ //! of text around to handle text insertion. use crate::pager::{PageRendering, Pager}; +use crate::threads::MainThread; use std::collections::LinkedList; use std::ffi::{CStr, CString}; use std::io::Write; @@ -176,7 +177,7 @@ pub struct Screen { pub autosuggestion_is_truncated: bool, /// Receiver for our output. - outp: &'static mut Outputter, + outp: &'static MainThread, /// The internal representation of the desired screen contents. desired: ScreenData, @@ -208,7 +209,7 @@ pub struct Screen { impl Screen { pub fn new() -> Self { Self { - outp: Outputter::stdoutput().get_mut(), + outp: Outputter::stdoutput(), autosuggestion_is_truncated: Default::default(), desired: Default::default(), actual: Default::default(), @@ -637,9 +638,9 @@ impl Screen { // Either issue a cr to go back to the beginning of this line, or a nl to go to the // beginning of the next one, depending on what we think is more efficient. if new_y <= zelf.actual.cursor.y { - zelf.outp.push(b'\r'); + zelf.outp.with_mut(|outp| outp.push(b'\r')); } else { - zelf.outp.push(b'\n'); + zelf.outp.with_mut(|outp| outp.push(b'\n')); zelf.actual.cursor.y += 1; } // Either way we're not in the first column. @@ -673,14 +674,16 @@ impl Screen { None }; - for _ in 0..y_steps.abs_diff(0) { - zelf.outp.tputs_if_some(&s); - } + zelf.outp.with_mut(|outp| { + for _ in 0..y_steps.abs_diff(0) { + outp.tputs_if_some(&s); + } + }); let mut x_steps = isize::try_from(new_x).unwrap() - isize::try_from(zelf.actual.cursor.x).unwrap(); if x_steps != 0 && new_x == 0 { - zelf.outp.push(b'\r'); + zelf.outp.with_mut(|outp| outp.push(b'\r')); x_steps = 0; } @@ -700,10 +703,10 @@ impl Screen { multi_str.as_ref().unwrap(), i32::try_from(x_steps.abs_diff(0)).unwrap(), ); - zelf.outp.tputs_if_some(&multi_param); + zelf.outp.with_mut(|outp| outp.tputs_if_some(&multi_param)); } else { for _ in 0..x_steps.abs_diff(0) { - zelf.outp.tputs_if_some(&s); + zelf.outp.with_mut(|outp| outp.tputs_if_some(&s)); } } @@ -715,7 +718,7 @@ impl Screen { fn write_char(&mut self, c: char, width: isize) { let mut zelf = self.scoped_buffer(); zelf.actual.cursor.x = zelf.actual.cursor.x.wrapping_add(width as usize); - zelf.outp.writech(c); + zelf.outp.with_mut(|outp| outp.writech(c)); if Some(zelf.actual.cursor.x) == zelf.actual.screen_width && allow_soft_wrap() { zelf.soft_wrap_location = Some(Cursor { x: 0, @@ -732,16 +735,16 @@ impl Screen { /// Send the specified string through tputs and append the output to the screen's outputter. fn write_mbs(&mut self, s: &CStr) { - self.outp.tputs(s) + self.outp.with_mut(|outp| outp.tputs(s)); } fn write_mbs_if_some(&mut self, s: &Option>) -> bool { - self.outp.tputs_if_some(s) + self.outp.with_mut(|outp| outp.tputs_if_some(s)) } /// Convert a wide string to a multibyte string and append it to the buffer. fn write_str(&mut self, s: &wstr) { - self.outp.write_wstr(s) + self.outp.with_mut(|outp| outp.write_wstr(s)); } /// Update the cursor as if soft wrapping had been performed. @@ -766,9 +769,9 @@ impl Screen { } fn scoped_buffer(&mut self) -> impl ScopeGuarding { - self.outp.begin_buffering(); + self.outp.with_mut(Outputter::begin_buffering); ScopeGuard::new(self, |zelf| { - zelf.outp.end_buffering(); + zelf.outp.with_mut(Outputter::end_buffering); }) } @@ -779,7 +782,7 @@ impl Screen { let mut set_color = |zelf: &mut Self, c| { let fg = color_resolver.resolve_spec(&c, false, vars); let bg = color_resolver.resolve_spec(&c, true, vars); - zelf.outp.set_color(fg, bg); + zelf.outp.with_mut(|outp| outp.set_color(fg, bg)); }; let mut cached_layouts = LAYOUT_CACHE_SHARED.lock().unwrap(); @@ -834,8 +837,9 @@ impl Screen { let mut start = 0; for line_break in left_prompt_layout.line_breaks { zelf.write_str(&left_prompt[start..line_break]); - zelf.outp - .tputs_if_some(&term.and_then(|term| term.clr_eol.as_ref())); + zelf.outp.with_mut(|outp| { + outp.tputs_if_some(&term.and_then(|term| term.clr_eol.as_ref())); + }); start = line_break; } zelf.write_str(&left_prompt[start..]); @@ -1067,9 +1071,9 @@ impl Screen { /// Issues an immediate clr_eos. pub fn screen_force_clear_to_end() { - Outputter::stdoutput() - .get_mut() - .tputs_if_some(&term().unwrap().clr_eos); + Outputter::stdoutput().with_mut(|outp| { + outp.tputs_if_some(&term().unwrap().clr_eos); + }); } /// Information about the layout of a prompt.