Merge PR #8928: fix: skip non-UTF-8 host env entries instead of panicking at startup (@STRML)

2026-08-19 01:39:17 +02:00
parent 78254059b7 46f31296a1
commit 9200fb3426
3 changed files with 261 additions and 38 deletions
@@ -587,6 +587,63 @@ async fn create_session(config: &ShellConfig) -> Result<ShellSessionCore> {
 	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<Item = (std::ffi::OsString, std::ffi::OsString)>,
+) -> Result<()> {
+	let mut merged_path: Option<String> = 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<Arc<process::SpawnRegistry>>,
@@ -643,42 +700,7 @@ async fn create_session_for_run(
 		}
 	}

-	let mut merged_path: Option<String> = 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 {
@@ -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<i32>, 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");
+}
@@ -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<Item = (String, String)> {
-	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");
+		}
+	}
 }