From ec9fc64ef571e169c6ee06782589467204049316 Mon Sep 17 00:00:00 2001 From: Filip Niksic Date: Fri, 14 Aug 2026 09:19:19 -0700 Subject: [PATCH] Serialize environment variable modifications in Rust tests via global mutex. In Google FuzzTest Rust tests, tests modifying environment variables previously required running in single-threaded mode via `--test-threads=1` to avoid data races across threads. Introduce an `EnvVars` builder utility that acquires a global mutex lock, applies key-value environment modifications, tracks pre-existing values, and restores original environment states when dropped. Remove `--test-threads=1` from Bazel test configurations, enabling full parallel test execution. PiperOrigin-RevId: 964734117 --- rust/BUILD | 2 - rust/options/BUILD | 2 - rust/options/src/lib.rs | 167 +++++++++++++++++++++++++++++++++++----- rust/src/options.rs | 53 +++---------- 4 files changed, 158 insertions(+), 66 deletions(-) diff --git a/rust/BUILD b/rust/BUILD index eb43c1107..8be2de2a2 100644 --- a/rust/BUILD +++ b/rust/BUILD @@ -48,8 +48,6 @@ rust_library( rust_test( name = "fuzztest_test", - # Avoid interference when setting/resetting environment variables in tests. - args = ["--test-threads=1"], crate = ":fuzztest", edition = "2024", rustc_flags = ["-Zallow-features=cfg_sanitize"], diff --git a/rust/options/BUILD b/rust/options/BUILD index 2fc95f343..0ec9cd904 100644 --- a/rust/options/BUILD +++ b/rust/options/BUILD @@ -34,8 +34,6 @@ rust_library( rust_test( name = "fuzztest_options_test", - # Avoid interference when setting/resetting environment variables in tests. - args = ["--test-threads=1"], crate = ":fuzztest_options", edition = "2024", deps = [ diff --git a/rust/options/src/lib.rs b/rust/options/src/lib.rs index 2e49538f6..3387e0fe8 100644 --- a/rust/options/src/lib.rs +++ b/rust/options/src/lib.rs @@ -17,6 +17,9 @@ use clap::{Parser, ValueEnum}; use humantime::Duration; +use std::collections::HashMap; +use std::ffi::{OsStr, OsString}; +use std::sync::{Mutex, MutexGuard}; /// Time budget calculation type for replay corpus mode. #[derive(ValueEnum, Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -172,6 +175,80 @@ pub struct ReplayCorpusOptions { pub time_budget_type: TimeBudgetType, } +static ENV_MUTEX: Mutex<()> = Mutex::new(()); + +/// Builder for safely setting and unsetting environment variables under a global lock in tests. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct EnvVars { + modifications: HashMap>, +} + +impl EnvVars { + /// Creates a new empty `EnvVars` builder. + pub fn new() -> Self { + Self::default() + } + + /// Specifies an environment variable to set. Overwrites any previous modification for `key`. + pub fn set(mut self, key: impl AsRef, val: impl AsRef) -> Self { + self.modifications.insert(key.as_ref().to_os_string(), Some(val.as_ref().to_os_string())); + self + } + + /// Specifies an environment variable to unset. Overwrites any previous modification for `key`. + pub fn unset(mut self, key: impl AsRef) -> Self { + self.modifications.insert(key.as_ref().to_os_string(), None); + self + } + + /// Locks the global environment mutex, records the original values of all modified variables, + /// applies the requested modifications, and returns an `EnvVarGuard` that restores original values + /// and unlocks the mutex when dropped. + pub fn lock(self) -> EnvVarGuard<'static> { + let guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + self.lock_with(guard) + } + + /// Applies environment variable modifications using an already acquired mutex guard. + pub fn lock_with<'a>(self, guard: MutexGuard<'a, ()>) -> EnvVarGuard<'a> { + let mut original_values = Vec::new(); + + for (key, target_state) in self.modifications { + original_values.push((key.clone(), std::env::var_os(&key))); + // SAFETY: Access to environment variables is serialized by holding the global mutex guard. + unsafe { + match target_state { + Some(val) => std::env::set_var(&key, val), + None => std::env::remove_var(&key), + } + } + } + + EnvVarGuard { _guard: guard, original_values } + } +} + +/// Guard managing modified environment variables. Restores original values when dropped. +#[must_use] +pub struct EnvVarGuard<'a> { + _guard: MutexGuard<'a, ()>, + original_values: Vec<(OsString, Option)>, +} + +impl<'a> Drop for EnvVarGuard<'a> { + fn drop(&mut self) { + for (key, orig) in &self.original_values { + // SAFETY: Access to environment variables is serialized by the global mutex held in self._guard. + unsafe { + match orig { + Some(val) => std::env::set_var(key, val), + None => std::env::remove_var(key), + } + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -180,42 +257,90 @@ mod tests { #[gtest] fn test_replay_id_requires_corpus_db() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_REPLAY_ID", "my_crash_123"); - std::env::remove_var("FUZZTEST_CORPUS_DB"); - } + let _guard = EnvVars::new() + .set("FUZZTEST_REPLAY_ID", "my_crash_123") + .unset("FUZZTEST_CORPUS_DB") + .lock(); let result = FuzzTestOptions::try_parse_from(std::iter::empty::()); - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_REPLAY_ID"); - } - let err = result.expect_err("parsing should fail when corpus_db is missing"); expect_that!(err.kind(), eq(clap::error::ErrorKind::MissingRequiredArgument)); } #[gtest] fn test_replay_id_with_corpus_db_succeeds() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_REPLAY_ID", "my_crash_123"); - std::env::set_var("FUZZTEST_CORPUS_DB", "/tmp/corpus_db"); - } + let _guard = EnvVars::new() + .set("FUZZTEST_REPLAY_ID", "my_crash_123") + .set("FUZZTEST_CORPUS_DB", "/tmp/corpus_db") + .lock(); let result = FuzzTestOptions::try_parse_from(std::iter::empty::()); - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_REPLAY_ID"); - std::env::remove_var("FUZZTEST_CORPUS_DB"); - } - let options = result.expect("parsing should succeed when both replay_id and corpus_db are present"); expect_that!(options.replay_id.as_deref(), eq(Some("my_crash_123"))); expect_that!(options.corpus_db.as_deref(), eq(Some("/tmp/corpus_db"))); } + + #[gtest] + fn test_modify_env_vars_restores_original_state() { + const PREEXISTING_KEY: &str = "FUZZTEST_TEST_VAR_PREEXISTING"; + const NEW_KEY: &str = "FUZZTEST_TEST_VAR_NEW"; + const UNSET_KEY: &str = "FUZZTEST_TEST_VAR_UNSET"; + + // Acquire global lock for the whole test duration so setup and cleanup are thread-safe. + let lock = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + + // Setup pre-existing state under lock + unsafe { + std::env::set_var(PREEXISTING_KEY, "initial_value"); + std::env::set_var(UNSET_KEY, "value_to_be_unset"); + std::env::remove_var(NEW_KEY); + } + + { + let _guard = EnvVars::new() + .set(PREEXISTING_KEY, "modified_value") + .set(NEW_KEY, "created_value") + .unset(UNSET_KEY) + .lock_with(lock); + + expect_that!(std::env::var(PREEXISTING_KEY), ok(eq("modified_value"))); + expect_that!(std::env::var(NEW_KEY), ok(eq("created_value"))); + expect_true!(std::env::var(UNSET_KEY).is_err()); + } + + // After guard drop, original state should be restored + expect_that!(std::env::var(PREEXISTING_KEY), ok(eq("initial_value"))); + expect_true!(std::env::var(NEW_KEY).is_err()); + expect_that!(std::env::var(UNSET_KEY), ok(eq("value_to_be_unset"))); + + // Cleanup + unsafe { + std::env::remove_var(PREEXISTING_KEY); + std::env::remove_var(UNSET_KEY); + } + } + + #[gtest] + fn test_env_vars_conflicting_modifications() { + const KEY: &str = "FUZZTEST_TEST_VAR_CONFLICT"; + + // Last modification wins: .set("val1").set("val2").unset() -> variable is unset + { + let _guard = EnvVars::new().set(KEY, "val1").set(KEY, "val2").unset(KEY).lock(); + + expect_true!(std::env::var(KEY).is_err()); + } + + // Last modification wins: .unset().set("final_val") -> variable is "final_val" + { + let _guard = EnvVars::new().unset(KEY).set(KEY, "final_val").lock(); + + expect_that!(std::env::var(KEY), ok(eq("final_val"))); + } + + expect_true!(std::env::var(KEY).is_err()); + } } diff --git a/rust/src/options.rs b/rust/src/options.rs index 0879705b9..71a2e19b9 100644 --- a/rust/src/options.rs +++ b/rust/src/options.rs @@ -23,8 +23,8 @@ use std::sync::OnceLock; use tempfile::{NamedTempFile, TempDir}; pub use fuzztest_options::{ - ExecutionMode, FuzzFor, FuzzOptions, FuzzTestOptions, ReplayCorpusOptions, ReplayCrashOptions, - TimeBudgetType, + EnvVarGuard, EnvVars, ExecutionMode, FuzzFor, FuzzOptions, FuzzTestOptions, + ReplayCorpusOptions, ReplayCrashOptions, TimeBudgetType, }; /// Returns a lazily-initialized static reference to the global `FuzzTestOptions`. @@ -355,10 +355,7 @@ mod tests { #[gtest] fn test_fuzz_options_parsing_env() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_FUZZ_FOR", "5s"); - } + let _guard = EnvVars::new().set("FUZZTEST_FUZZ_FOR", "5s").lock(); let options = FuzzTestOptions::parse_from(std::iter::empty::()); @@ -367,19 +364,11 @@ mod tests { ExecutionMode::from_fuzztest_options(&options), matches_pattern!(ExecutionMode::Fuzz(_)) ); - - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_FUZZ_FOR"); - } } #[gtest] fn test_fuzz_options_parsing_indefinite_env() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_FUZZ_FOR", "inf"); - } + let _guard = EnvVars::new().set("FUZZTEST_FUZZ_FOR", "inf").lock(); let options = FuzzTestOptions::parse_from(std::iter::empty::()); @@ -389,11 +378,6 @@ mod tests { panic!("Expected ExecutionMode::Fuzz"); }; expect_that!(fuzz_opts.fuzz_for, eq(FuzzFor::Indefinitely)); - - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_FUZZ_FOR"); - } } #[gtest] @@ -670,39 +654,26 @@ mod tests { #[gtest] fn test_replay_id_requires_corpus_db() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_REPLAY_ID", "my_crash_123"); - std::env::remove_var("FUZZTEST_CORPUS_DB"); - } + let _guard = EnvVars::new() + .set("FUZZTEST_REPLAY_ID", "my_crash_123") + .unset("FUZZTEST_CORPUS_DB") + .lock(); let result = FuzzTestOptions::try_parse_from(std::iter::empty::()); - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_REPLAY_ID"); - } - let err = result.expect_err("parsing should fail when corpus_db is missing"); expect_that!(err.kind(), eq(clap::error::ErrorKind::MissingRequiredArgument)); } #[gtest] fn test_replay_id_with_corpus_db_succeeds() { - // SAFETY: Testing environment parsing in single-threaded context. - unsafe { - std::env::set_var("FUZZTEST_REPLAY_ID", "my_crash_123"); - std::env::set_var("FUZZTEST_CORPUS_DB", "/tmp/corpus_db"); - } + let _guard = EnvVars::new() + .set("FUZZTEST_REPLAY_ID", "my_crash_123") + .set("FUZZTEST_CORPUS_DB", "/tmp/corpus_db") + .lock(); let result = FuzzTestOptions::try_parse_from(std::iter::empty::()); - // SAFETY: Cleaning up environment variables. - unsafe { - std::env::remove_var("FUZZTEST_REPLAY_ID"); - std::env::remove_var("FUZZTEST_CORPUS_DB"); - } - let options = result.expect("parsing should succeed when both replay_id and corpus_db are present"); expect_that!(options.replay_id.as_deref(), eq(Some("my_crash_123")));