Extract some setlocale() calls; use C-string literals

src/bin/fish.rs

@@ -41,6 +41,7 @@
     fprintf, function, future_feature_flags as features,
     history::{self, start_private_mode},
     io::IoChain,
+    locale::set_libc_locales,
     nix::{RUsage, getpid, getrusage, isatty},
     panic::panic_handler,
     parse_constants::{ParseErrorList, ParseTreeFlags},
@@ -63,7 +64,7 @@
 use libc::STDIN_FILENO;
 #[cfg(feature = "embed-data")]
 use rust_embed::RustEmbed;
-use std::ffi::{CString, OsStr, OsString};
+use std::ffi::{OsStr, OsString};
 use std::fs::File;
 use std::os::unix::prelude::*;
 use std::path::Path;
@@ -406,12 +407,8 @@ fn throwing_main() -> i32 {
     topic_monitor::topic_monitor_init();
     threads::init();
 
-    {
-        let s = CString::new("").unwrap();
-        unsafe {
-            libc::setlocale(libc::LC_ALL, s.as_ptr());
-        }
-    }
+    // Safety: single-threaded.
+    unsafe { set_libc_locales() };
 
     fish::wutil::gettext::initialize_gettext();
 

src/builtins/fish_indent.rs

@@ -1,12 +1,12 @@
 //! The fish_indent program.
 
-use std::ffi::{CString, OsStr};
+use std::ffi::OsStr;
 use std::fs;
 use std::io::{Read, Write};
 use std::os::unix::ffi::OsStrExt;
 
+use crate::locale::set_libc_locales;
 use crate::panic::panic_handler;
-use libc::LC_ALL;
 
 use super::prelude::*;
 use crate::ast::{self, Ast, Kind, Leaf, Node, NodeVisitor, SourceRangeList, Traversal};
@@ -892,10 +892,8 @@ fn throwing_main() -> i32 {
     // Using the user's default locale could be a problem if it doesn't use UTF-8 encoding. That's
     // because the fish project assumes Unicode UTF-8 encoding in all of its scripts.
     //
-    {
-        let s = CString::new("").unwrap();
-        unsafe { libc::setlocale(LC_ALL, s.as_ptr()) };
-    }
+    // Safety: single-threaded.
+    unsafe { set_libc_locales() };
     crate::wutil::gettext::initialize_gettext();
     env_init(None, true, false);
 

src/env_dispatch.rs

@@ -3,6 +3,7 @@
 use crate::env::{EnvMode, EnvStack, Environment, setenv_lock, unsetenv_lock};
 use crate::flog::FLOG;
 use crate::input_common::{update_wait_on_escape_ms, update_wait_on_sequence_key_ms};
+use crate::locale::set_libc_locales;
 use crate::reader::{
     reader_change_cursor_end_mode, reader_change_cursor_selection_mode, reader_change_history,
     reader_schedule_prompt_repaint, reader_set_autosuggestion_enabled, reader_set_transient_prompt,
@@ -526,18 +527,16 @@ fn init_locale(vars: &EnvStack) {
     }
 
     let user_locale = {
-        let loc_ptr = unsafe { libc::setlocale(libc::LC_ALL, c"".as_ptr().cast()) };
-        if loc_ptr.is_null() {
+        // Safety: we hold the locale lock.
+        let loc = unsafe { set_libc_locales() };
+        if loc.is_none() {
             FLOG!(env_locale, "user has an invalid locale configured");
-            None
-        } else {
-            // safety: setlocale did not return a null-pointer, so it is a valid pointer
-            Some(unsafe { CStr::from_ptr(loc_ptr) })
         }
+        loc
     };
 
     // We *always* use a C-locale for numbers because we want '.' (except for in printf).
-    let loc_ptr = unsafe { libc::setlocale(libc::LC_NUMERIC, c"C".as_ptr().cast()) };
+    let loc_ptr = unsafe { libc::setlocale(libc::LC_NUMERIC, c"C".as_ptr()) };
     // should never fail, the C locale should always be defined
     assert_ne!(loc_ptr, ptr::null_mut());
 

src/locale.rs

@@ -1,10 +1,27 @@
 /// Support for the "current locale."
 pub use fish_printf::locale::{C_LOCALE, Locale};
-use std::sync::Mutex;
+use std::{ffi::CStr, sync::Mutex};
 
 /// Lock guarding libc `setlocale()` or `localeconv()` calls to avoid races.
 pub(crate) static LOCALE_LOCK: Mutex<()> = Mutex::new(());
 
+/// # Safety
+/// Call this either before starting any locale-using thread, or while holding a lock on the
+/// above mutex.
+pub unsafe fn set_libc_locales() -> Option<&'static CStr> {
+    setlocale(libc::LC_ALL, Some(c""))
+}
+
+fn setlocale(category: libc::c_int, locale: Option<&CStr>) -> Option<&'static CStr> {
+    let loc_ptr = {
+        let locale = locale.map_or(std::ptr::null(), |loc| loc.as_ptr());
+        unsafe { libc::setlocale(category, locale) }
+    };
+    (!loc_ptr.is_null()).then(||
+        // Safety: setlocale did not return a null-pointer, so it is a valid pointer
+        unsafe{CStr::from_ptr(loc_ptr)})
+}
+
 /// It's CHAR_MAX.
 const CHAR_MAX: libc::c_char = libc::c_char::MAX;
 
@@ -67,13 +84,10 @@ unsafe fn read_locale() -> Option<Locale> {
         unsafe fn localeconv_l(loc: libc::locale_t) -> *const libc::lconv;
     }
 
-    const empty: [libc::c_char; 1] = [0];
-
     // We create a new locale (pass 0 locale_t base)
     // and pass no "locale", so everything else is taken from the environment.
     // This is fine because we're only using this for numbers.
-    let loc =
-        unsafe { libc::newlocale(libc::LC_NUMERIC_MASK, empty.as_ptr(), 0 as libc::locale_t) };
+    let loc = unsafe { libc::newlocale(libc::LC_NUMERIC_MASK, c"".as_ptr(), 0 as libc::locale_t) };
     if loc.is_null() {
         return None;
     }
@@ -94,11 +108,9 @@ unsafe fn read_locale() -> Option<Locale> {
     // Bleh, we have to go through localeconv, which races with setlocale.
     // TODO: There has to be a better way to do this.
     let _guard = LOCALE_LOCK.lock().unwrap();
-    const empty: [libc::c_char; 1] = [0];
-    const c_loc_str: [libc::c_char; 2] = [b'C' as libc::c_char, 0];
 
     unsafe {
-        libc::setlocale(libc::LC_NUMERIC, empty.as_ptr());
+        libc::setlocale(libc::LC_NUMERIC, c"".as_ptr());
     }
 
     let lconv = unsafe { libc::localeconv() };
@@ -109,7 +121,7 @@ unsafe fn read_locale() -> Option<Locale> {
     };
     // Note we *always* use a C-locale for numbers, because we always want "." except for in printf.
     unsafe {
-        libc::setlocale(libc::LC_NUMERIC, c_loc_str.as_ptr());
+        libc::setlocale(libc::LC_NUMERIC, c"C".as_ptr());
     }
     result
 }

src/tests/prelude.rs

@@ -1,6 +1,7 @@
 use crate::common::{BUILD_DIR, ScopeGuard, ScopeGuarding};
 use crate::env::env_init;
 use crate::env::{EnvMode, EnvVar, EnvVarFlags, Environment};
+use crate::locale::set_libc_locales;
 use crate::parser::{CancelBehavior, Parser};
 use crate::reader::{reader_deinit, reader_init};
 use crate::signal::signal_reset_handlers;
@@ -12,7 +13,6 @@
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::env::set_current_dir;
-use std::ffi::CString;
 use std::path::PathBuf;
 
 pub use serial_test::serial;
@@ -26,12 +26,8 @@ pub fn test_init() -> impl ScopeGuarding<Target = ()> {
         test_dir.push("fish-test");
         std::fs::create_dir_all(&test_dir).unwrap();
         set_current_dir(&test_dir).unwrap();
-        {
-            let s = CString::new("").unwrap();
-            unsafe {
-                libc::setlocale(libc::LC_ALL, s.as_ptr());
-            }
-        }
+        // Safety: all tests that access locale should go through the enclosing function.
+        unsafe { set_libc_locales() };
         topic_monitor_init();
         crate::threads::init();
         proc_init();