diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 624acf68a..181e2a4fd 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -587,6 +587,63 @@ async fn create_session(config: &ShellConfig) -> Result { create_session_for_run(config, None, None).await } +/// Copies the host environment into `shell`, merging duplicate `PATH` values +/// and registering the merged `PATH` last. +/// +/// Entries whose key or value is not valid Unicode are skipped: a corrupt +/// entry carries no usable meaning, and `std::env::vars()` — the naive way to +/// read the host environment — panics on the first one before any command can +/// run. `vars_os()` yields the raw entries without panicking, leaving the +/// skip decision here. +fn copy_env_into_shell( + shell: &mut BrushShell, + env: impl Iterator, +) -> Result<()> { + let mut merged_path: Option = None; + for (key, value) in env { + // A key or value that cannot be decoded as Unicode is unusable; drop + // it rather than panicking startup for a corrupt host environment. + let (Some(key), Some(value)) = (key.to_str(), value.to_str()) else { + continue; + }; + let normalized_key = normalize_env_key(key); + if should_skip_env_var(normalized_key) { + continue; + } + if normalized_key == "PATH" { + merged_path = Some(match merged_path { + Some(existing) => merge_path_values(&existing, value), + None => value.to_string(), + }); + continue; + } + let mut var = ShellVariable::new(ShellValue::String(value.to_string())); + var.export(); + shell + .env_mut() + .set_global(normalized_key, var) + .map_err(|err| Error::msg(format!("Failed to set env: {err}")))?; + } + + #[cfg(windows)] + if merged_path.is_none() + && let Some(value) = std::env::var_os("Path").or_else(|| std::env::var_os("PATH")) + { + merged_path = Some(value.to_string_lossy().into_owned()); + } + + if let Some(path_value) = &merged_path { + let mut var = ShellVariable::new(ShellValue::String(path_value.clone())); + var.export(); + shell + .env_mut() + .set_global("PATH", var) + .map_err(|err| Error::msg(format!("Failed to set env: {err}")))?; + } + + Ok(()) +} + async fn create_session_for_run( config: &ShellConfig, spawn_registry: Option>, @@ -643,42 +700,7 @@ async fn create_session_for_run( } } - let mut merged_path: Option = None; - for (key, value) in std::env::vars() { - let normalized_key = normalize_env_key(&key); - if should_skip_env_var(normalized_key) { - continue; - } - if normalized_key == "PATH" { - merged_path = Some(match merged_path { - Some(existing) => merge_path_values(&existing, &value), - None => value, - }); - continue; - } - let mut var = ShellVariable::new(ShellValue::String(value)); - var.export(); - shell - .env_mut() - .set_global(normalized_key, var) - .map_err(|err| Error::msg(format!("Failed to set env: {err}")))?; - } - - #[cfg(windows)] - if merged_path.is_none() - && let Some(value) = std::env::var_os("Path").or_else(|| std::env::var_os("PATH")) - { - merged_path = Some(value.to_string_lossy().into_owned()); - } - - if let Some(path_value) = &merged_path { - let mut var = ShellVariable::new(ShellValue::String(path_value.clone())); - var.export(); - shell - .env_mut() - .set_global("PATH", var) - .map_err(|err| Error::msg(format!("Failed to set env: {err}")))?; - } + copy_env_into_shell(&mut shell, std::env::vars_os())?; if let Some(env) = config.session_env.as_ref() { for (key, value) in env { @@ -1973,6 +1995,58 @@ mod tests { (result, output) } + /// Regression for issue #8925: a host env entry whose key or value is not + /// valid Unicode must be skipped, not fatal. `std::env::vars()` panics on + /// the first one (e.g. the corrupt `GHOSTTY_BIN_DIR` cmux/Ghostty stages, + /// bytes `9d d9 50`) before any command runs; `copy_env_into_shell` reads + /// via `vars_os()` and drops corrupt entries while still copying the rest + /// and merging duplicate `PATH` values. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn copy_env_skips_non_utf8_entries_and_merges_path() { + use std::os::unix::ffi::OsStringExt; + + let mut shell = BrushShell::builder() + .do_not_inherit_env(true) + .profile(ProfileLoadBehavior::Skip) + .rc(RcLoadBehavior::Skip) + .builtins(default_builtins(BuiltinSet::BashMode)) + .build() + .await + .expect("build shell"); + + // GHOSTTY_BIN_DIR with the corrupt bytes, a normal var, a corrupt key, + // and a duplicate PATH — in host-env iteration order. + let entries = vec![ + (std::ffi::OsString::from("PATH"), std::ffi::OsString::from("/usr/bin:/bin")), + ( + std::ffi::OsString::from("GHOSTTY_BIN_DIR"), + std::ffi::OsString::from_vec(vec![0x9d, 0xd9, 0x50]), + ), + (std::ffi::OsString::from("HOME"), std::ffi::OsString::from("/home/tester")), + ( + std::ffi::OsString::from_vec(vec![0xff, b'B', b'A', b'D']), + std::ffi::OsString::from("x"), + ), + (std::ffi::OsString::from("PATH"), std::ffi::OsString::from("/opt/bin")), + ]; + copy_env_into_shell(&mut shell, entries.into_iter()).expect("copy host env"); + + let value = |name: &str| { + shell + .env() + .get(name) + .and_then(|(_, var)| match var.value() { + ShellValue::String(value) => Some(value.clone()), + _ => None, + }) + }; + assert_eq!(value("PATH").as_deref(), Some("/opt/bin"), "PATH is merged/preserved"); + assert_eq!(value("HOME").as_deref(), Some("/home/tester"), "valid entry copied"); + assert!(value("GHOSTTY_BIN_DIR").is_none(), "non-UTF-8 value must be skipped"); + assert!(value("BAD").is_none(), "non-UTF-8 key must be skipped"); + } + #[cfg(unix)] async fn wait_for_process_name(pid: i32, expected: &str) { time::timeout(Duration::from_secs(2), async { diff --git a/crates/pi-shell/tests/nonutf8_env.rs b/crates/pi-shell/tests/nonutf8_env.rs new file mode 100644 index 000000000..5c3406bc2 --- /dev/null +++ b/crates/pi-shell/tests/nonutf8_env.rs @@ -0,0 +1,98 @@ +//! Regression tests for oh-my-pi issue #8925: a host environment entry whose +//! key or value is not valid Unicode must not crash session startup or command +//! execution. +//! +//! This lives in `tests/` (a process of its own) on purpose: `set_var` mutates +//! the process-global environment, and a corrupt entry would poison any other +//! test binary that still reads it via `std::env::vars()`. + +#![cfg(unix)] + +use std::os::unix::ffi::OsStrExt; + +use pi_shell::{ShellExecuteOptions, cancel::CancelToken, execute_shell}; + +/// The corrupt bytes cmux/Ghostty staged as `GHOSTTY_BIN_DIR` on the +/// reporter's host: `9d d9 50` has no valid UTF-8 encoding. +const GHOSTTY_BIN_DIR_BYTES: &[u8] = &[0x9d, 0xd9, 0x50]; + +/// Sets then restores a process-global env var, scoped to the test body. +struct ScopedEnvVar { + key: &'static str, +} + +impl ScopedEnvVar { + fn set_corrupt(key: &'static str, bytes: &[u8]) -> Self { + // SAFETY: this dedicated test process mutates the environment only + // here, between awaits, so no other thread is reading or writing it + // concurrently. + unsafe { std::env::set_var(key, std::ffi::OsStr::from_bytes(bytes)) }; + Self { key } + } + + fn set(key: &'static str, value: &str) -> Self { + // SAFETY: as in `set_corrupt`; the process-global environment is + // mutated only at these controlled points. + unsafe { std::env::set_var(key, value) }; + Self { key } + } +} + +impl Drop for ScopedEnvVar { + fn drop(&mut self) { + // SAFETY: the guard is dropped after all awaits have completed, when + // no runtime thread is reading the environment. + unsafe { std::env::remove_var(self.key) }; + } +} + +/// Runs one shell command through the public one-shot entry point and returns +/// its exit code plus captured stdout. +async fn run(command: &str) -> (Option, String) { + let (tx, rx) = flume::unbounded(); + let result = execute_shell( + ShellExecuteOptions { command: command.to_string(), ..Default::default() }, + Some(tx), + CancelToken::default(), + ) + .await + .expect("shell execution"); + let output = rx.try_iter().collect(); + (result.exit_code, output) +} + +/// The session (re)created by each `execute_shell` call copies the host +/// environment key by key. A corrupt value must be skipped, not panicked over, +/// while other entries (here the sentinel and PATH) still land. +#[tokio::test(flavor = "multi_thread")] +async fn session_start_skips_non_utf8_value_and_preserves_env() { + let path = std::env::var("PATH").unwrap_or_default(); + + let _corrupt = ScopedEnvVar::set_corrupt("OMP_TEST_CORRUPT_8925", GHOSTTY_BIN_DIR_BYTES); + let _sentinel = ScopedEnvVar::set("OMP_TEST_SENTINEL_8925", "sentinel-value"); + + // Starts with a corrupt var present: must not panic. + let (code, output) = run("echo $OMP_TEST_SENTINEL_8925").await; + assert_eq!(code, Some(0), "session start must succeed with a corrupt env var"); + assert!( + output.contains("sentinel-value"), + "valid env entries must still be copied; got {output:?}" + ); + + // PATH survives the copy unchanged. + let (code, output) = run("echo \"$PATH\"").await; + assert_eq!(code, Some(0)); + assert_eq!(output.trim_end(), path, "PATH must survive the corrupt-env copy"); +} + +/// The process builtins (`sleep`, `timeout`, `pgrep`, …) each build a plain +/// brush shell that inherits the host environment via brush-core's +/// `get_host_env_vars`, the second `std::env::vars()` sink. Must not panic +/// either. +#[tokio::test(flavor = "multi_thread")] +async fn process_builtin_shell_build_survives_non_utf8_env() { + let _corrupt = ScopedEnvVar::set_corrupt("OMP_TEST_CORRUPT_8925", GHOSTTY_BIN_DIR_BYTES); + + let (code, _) = run("sleep 0").await; + assert_eq!(code, Some(0), "process builtin must survive a corrupt env var"); +} diff --git a/crates/vendor/brush-core/src/sys/unix/env.rs b/crates/vendor/brush-core/src/sys/unix/env.rs index 6330ca06f..348e7c7f0 100644 --- a/crates/vendor/brush-core/src/sys/unix/env.rs +++ b/crates/vendor/brush-core/src/sys/unix/env.rs @@ -2,7 +2,58 @@ /// Retrieves environment variables from the host process. /// -/// On Unix, this is a direct passthrough to [`std::env::vars()`]. +/// This is a best-effort passthrough to [`std::env::vars()`], skipping entries +/// whose key or value is not valid Unicode: a corrupt entry carries no usable +/// meaning, and a naive [`std::env::vars()`] call panics on the first one +/// before any command can run. pub(crate) fn get_host_env_vars() -> impl Iterator { - std::env::vars() + std::env::vars_os().filter_map(|(key, value)| { + let key = key.into_string().ok()?; + let value = value.into_string().ok()?; + Some((key, value)) + }) +} + +#[cfg(test)] +mod tests { + use std::os::unix::ffi::OsStrExt; + + use super::*; + + /// Regression for oh-my-pi #8925: `std::env::vars()` panics on a key or + /// value that is not valid Unicode (e.g. the corrupt `GHOSTTY_BIN_DIR` + /// delivered by cmux/Ghostty, bytes `9d d9 50`). Shells that call this to + /// inherit the host environment must skip such entries, never crash. + #[test] + fn get_host_env_vars_skips_non_utf8_entries() { + // SAFETY: the corrupt + sentinel keys are unique to this test and the + // environment is not contended elsewhere in this process. + unsafe { + std::env::set_var("OMP_TEST_BAD_VALUE_8925", std::ffi::OsStr::from_bytes(&[0x9d, 0xd9, 0x50])); + std::env::set_var(std::ffi::OsStr::from_bytes(b"\xffOMP_TEST_BAD_KEY_8925"), "value"); + std::env::set_var("OMP_TEST_KEEP_8925", "kept"); + } + + let entries: Vec<(String, String)> = get_host_env_vars().collect(); + + assert!( + !entries.iter().any(|(key, _)| key == "OMP_TEST_BAD_VALUE_8925"), + "a non-UTF-8 value must be skipped" + ); + assert!( + !entries.iter().any(|(key, _)| key.contains("OMP_TEST_BAD_KEY")), + "a non-UTF-8 key must be skipped" + ); + assert!( + entries.iter().any(|(key, value)| key == "OMP_TEST_KEEP_8925" && value == "kept"), + "valid entries must still be returned" + ); + + // SAFETY: reads are done; restore the host environment. + unsafe { + std::env::remove_var("OMP_TEST_BAD_VALUE_8925"); + std::env::remove_var(std::ffi::OsStr::from_bytes(b"\xffOMP_TEST_BAD_KEY_8925")); + std::env::remove_var("OMP_TEST_KEEP_8925"); + } + } }