Fix safety issues with some static variables (fish-shell#10329)

Add safe Send/Sync wrapper for main thread data

Author: mqudsi

src/output.rs

@@ -3,6 +3,7 @@ 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;
@@ -444,14 +445,10 @@ impl Outputter {
 
     /// Access the outputter for stdout.
     /// This should only be used from the main thread.
-    pub fn stdoutput() -> &'static mut RefCell<Outputter> {
-        crate::threads::assert_is_main_thread();
-        static mut STDOUTPUT: RefCell<Outputter> =
-            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 RefCell<Outputter> {
+        static STDOUTPUT: MainThread<RefCell<Outputter>> =
+            MainThread::new(RefCell::new(Outputter::new_from_fd(libc::STDOUT_FILENO)));
+        STDOUTPUT.get()
     }
 }
 

src/parser.rs

@@ -27,7 +27,7 @@ use crate::parse_execution::{EndExecutionReason, ParseExecutionContext};
 use crate::parse_tree::{parse_source, ParsedSourceRef};
 use crate::proc::{job_reap, JobGroupRef, JobList, JobRef, ProcStatus};
 use crate::signal::{signal_check_cancel, signal_clear_cancel, Signal};
-use crate::threads::assert_is_main_thread;
+use crate::threads::{assert_is_main_thread, MainThread};
 use crate::util::get_time;
 use crate::wait_handle::WaitHandleStore;
 use crate::wchar::{wstr, WString, L};
@@ -414,19 +414,11 @@ impl Parser {
         false
     }
 
-    /// Get the "principal" parser, whatever that is.
+    /// Get the "principal" parser, whatever that is. Can only be called by the main thread.
     pub fn principal_parser() -> &'static Parser {
-        // XXX: We use `static mut` as a hack to work around the fact that Parser doesn't implement
-        // Sync! Even though we are wrapping it in Lazy<> and it compiles without an error, that
-        // doesn't mean this is safe to access across threads!
-        static mut PRINCIPAL: Lazy<ParserRef> =
-            Lazy::new(|| Parser::new(EnvStack::principal().clone(), true));
-        // XXX: Creating and using multiple (read or write!) references to the same mutable static
-        // is undefined behavior!
-        unsafe {
-            PRINCIPAL.assert_can_execute();
-            &PRINCIPAL
-        }
+        static PRINCIPAL: Lazy<MainThread<ParserRef>> =
+            Lazy::new(|| MainThread::new(Parser::new(EnvStack::principal().clone(), true)));
+        PRINCIPAL.get()
     }
 
     /// Assert that this parser is allowed to execute on the current thread.

src/reader.rs

@@ -1906,7 +1906,7 @@ impl ReaderData {
                 perror("tcsetattr"); // return to previous mode
             }
             Outputter::stdoutput()
-                .get_mut()
+                .borrow_mut()
                 .set_color(RgbColor::RESET, RgbColor::RESET);
         }
         rls.finished.then(|| zelf.command_line.text().to_owned())
@@ -2946,8 +2946,9 @@ impl ReaderData {
                 el.end_edit_group();
             }
             rl::DisableMouseTracking => {
-                let outp = Outputter::stdoutput().get_mut();
-                outp.write_wstr(L!("\x1B[?1000l"));
+                Outputter::stdoutput()
+                    .borrow_mut()
+                    .write_wstr(L!("\x1B[?1000l"));
             }
             rl::ClearScreenAndRepaint => {
                 self.parser().libdata_mut().pods.is_repaint = true;
@@ -2958,8 +2959,7 @@ 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().borrow_mut().write_wstr(&clear);
                     self.screen.reset_line(/*repaint_prompt=*/ true);
                     self.layout_and_repaint(L!("readline"));
                 }
@@ -3492,7 +3492,7 @@ fn reader_interactive_init(parser: &Parser) {
 /// Destroy data for interactive use.
 fn reader_interactive_destroy() {
     Outputter::stdoutput()
-        .get_mut()
+        .borrow_mut()
         .set_color(RgbColor::RESET, RgbColor::RESET);
 }
 
@@ -3575,7 +3575,7 @@ pub fn reader_write_title(
     }
 
     Outputter::stdoutput()
-        .get_mut()
+        .borrow_mut()
         .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).
@@ -4587,9 +4587,10 @@ 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()
+        .borrow_mut()
+        .set_color(RgbColor::NORMAL, RgbColor::NORMAL);
     term_donate(false);
 
     let time_before = Instant::now();

src/screen.rs

@@ -8,6 +8,7 @@
 //! of text around to handle text insertion.
 
 use crate::pager::{PageRendering, Pager};
+use std::cell::RefCell;
 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 RefCell<Outputter>,
 
     /// 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.borrow_mut().push(b'\r');
             } else {
-                zelf.outp.push(b'\n');
+                zelf.outp.borrow_mut().push(b'\n');
                 zelf.actual.cursor.y += 1;
             }
             // Either way we're not in the first column.
@@ -674,13 +675,13 @@ impl Screen {
         };
 
         for _ in 0..y_steps.abs_diff(0) {
-            zelf.outp.tputs_if_some(&s);
+            zelf.outp.borrow_mut().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.borrow_mut().push(b'\r');
             x_steps = 0;
         }
 
@@ -700,10 +701,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.borrow_mut().tputs_if_some(&multi_param);
         } else {
             for _ in 0..x_steps.abs_diff(0) {
-                zelf.outp.tputs_if_some(&s);
+                zelf.outp.borrow_mut().tputs_if_some(&s);
             }
         }
 
@@ -715,7 +716,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.borrow_mut().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 +733,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.borrow_mut().tputs(s);
     }
 
     fn write_mbs_if_some(&mut self, s: &Option<impl AsRef<CStr>>) -> bool {
-        self.outp.tputs_if_some(s)
+        self.outp.borrow_mut().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.borrow_mut().write_wstr(s);
     }
 
     /// Update the cursor as if soft wrapping had been performed.
@@ -766,9 +767,9 @@ impl Screen {
     }
 
     fn scoped_buffer(&mut self) -> impl ScopeGuarding<Target = &mut Screen> {
-        self.outp.begin_buffering();
+        self.outp.borrow_mut().begin_buffering();
         ScopeGuard::new(self, |zelf| {
-            zelf.outp.end_buffering();
+            zelf.outp.borrow_mut().end_buffering();
         })
     }
 
@@ -779,7 +780,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.borrow_mut().set_color(fg, bg);
         };
 
         let mut cached_layouts = LAYOUT_CACHE_SHARED.lock().unwrap();
@@ -835,6 +836,7 @@ impl Screen {
             for line_break in left_prompt_layout.line_breaks {
                 zelf.write_str(&left_prompt[start..line_break]);
                 zelf.outp
+                    .borrow_mut()
                     .tputs_if_some(&term.and_then(|term| term.clr_eol.as_ref()));
                 start = line_break;
             }
@@ -1068,7 +1070,7 @@ impl Screen {
 /// Issues an immediate clr_eos.
 pub fn screen_force_clear_to_end() {
     Outputter::stdoutput()
-        .get_mut()
+        .borrow_mut()
         .tputs_if_some(&term().unwrap().clr_eos);
 }
 

src/threads.rs

@@ -4,6 +4,7 @@
 use crate::flog::{FloggableDebug, FLOG};
 use crate::reader::ReaderData;
 use once_cell::race::OnceBox;
+use std::marker::PhantomData;
 use std::num::NonZeroU64;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex};
@@ -358,6 +359,33 @@ impl ThreadPool {
     }
 }
 
+/// A `Sync` and `Send` wrapper for non-`Sync`/`Send` types.
+/// Only allows access from the main thread.
+pub struct MainThread<T> {
+    data: T,
+    // Make type !Send and !Sync by default
+    _marker: PhantomData<*const ()>,
+}
+
+// Manually implement Send and Sync for MainThread<T> to ensure it can be shared across threads
+// as long as T is 'static.
+unsafe impl<T: 'static> Send for MainThread<T> {}
+unsafe impl<T: 'static> Sync for MainThread<T> {}
+
+impl<T> MainThread<T> {
+    pub const fn new(value: T) -> Self {
+        Self {
+            data: value,
+            _marker: PhantomData,
+        }
+    }
+
+    pub fn get(&self) -> &T {
+        assert_is_main_thread();
+        &self.data
+    }
+}
+
 pub struct WorkerThread {
     /// The data shared with the [`ThreadPool`].
     shared: Arc<ThreadPoolShared>,