From 60fbfeec80e726df2c15aae9a3df30da0f30bddb Mon Sep 17 00:00:00 2001 From: "Sami Daniel (Tsoi)" Date: Sun, 1 Jun 2025 02:15:13 -0300 Subject: [PATCH 1/2] matches/exec: create path integrity check before execution Fixes #518 Create function check_path_integrity which checks the integrity of the path, with the following criteria: - Only absolute paths - Only non-empty paths - Only directories in path If the PATH env var does not pass the criteria, an error is returned warning about the specific path part that caused that error. Tests: - Added tests for both SingleExecMatcher and MultiExecMatcher - Covered all PATH validation scenarios: * Valid absolute directories * Empty path segments * Relative path segments * File paths instead of directories - Ensured safe environment variable handling with unsafe blocks - Maintained consistent test patterns with existing serial tests - Verified correct error handling for invalid PATH configurations To avoid making the function that performs the check public, tests were also added that verify that there is no error with a valid PATH. --- src/find/matchers/exec.rs | 30 ++++ tests/exec_unit_tests.rs | 346 +++++++++++++++++++++++++++++++++++++- 2 files changed, 374 insertions(+), 2 deletions(-) diff --git a/src/find/matchers/exec.rs b/src/find/matchers/exec.rs index 51337953..4741cf5d 100644 --- a/src/find/matchers/exec.rs +++ b/src/find/matchers/exec.rs @@ -5,6 +5,7 @@ // https://opensource.org/licenses/MIT. use std::cell::RefCell; +use std::env; use std::error::Error; use std::ffi::OsString; use std::io::{stderr, Write}; @@ -13,6 +14,27 @@ use std::process::Command; use super::{Matcher, MatcherIO, WalkEntry}; +fn check_path_integrity() -> Result<(), Box> { + let path_dirs = env::var("PATH")?; + for dir_entry in env::split_paths(&path_dirs) { + // We can securely unwrap (or expect) the value of dir_entry string + // conversion on message error cause the env::var returns an VarError + // variant that indicates if the variable (in this case PATH) contains + // invalid Unicode data. + let dir_entry_str = dir_entry.to_str().expect("Unexpected conversion error"); + if !dir_entry.is_absolute() || dir_entry.is_file() || dir_entry_str.is_empty() { + return Err(format!( + "The PATH environment variable contains non-absolute paths, \ + files, or empty paths. Segment that caused the error: '{}'", + dir_entry_str + ) + .into()); + } + } + + Ok(()) +} + enum Arg { FileArg(Vec), LiteralArg(OsString), @@ -30,6 +52,10 @@ impl SingleExecMatcher { args: &[&str], exec_in_parent_dir: bool, ) -> Result> { + if exec_in_parent_dir { + check_path_integrity()?; + } + let transformed_args = args .iter() .map(|&a| { @@ -112,6 +138,10 @@ impl MultiExecMatcher { args: &[&str], exec_in_parent_dir: bool, ) -> Result> { + if exec_in_parent_dir { + check_path_integrity()?; + } + let transformed_args = args.iter().map(OsString::from).collect(); Ok(Self { diff --git a/tests/exec_unit_tests.rs b/tests/exec_unit_tests.rs index e952e405..3285253b 100644 --- a/tests/exec_unit_tests.rs +++ b/tests/exec_unit_tests.rs @@ -8,11 +8,12 @@ /// ! But as the tests require running an external executable, they need to be run /// ! as integration tests so we can ensure that our testing-commandline binary /// ! has been built. +use serial_test::serial; use std::env; use std::fs::File; -use std::io::Read; +use std::io::{Read, Write}; use std::path::Path; -use tempfile::Builder; +use tempfile::{tempdir, Builder}; use common::test_helpers::{ fix_up_slashes, get_dir_entry_for, path_to_testing_commandline, FakeDependencies, @@ -23,6 +24,7 @@ use findutils::find::matchers::{Matcher, MatcherIO}; mod common; #[test] +#[serial(path_addicted)] fn matching_executes_code() { let temp_dir = Builder::new() .prefix("matching_executes_code") @@ -54,6 +56,7 @@ fn matching_executes_code() { } #[test] +#[serial(path_addicted)] fn matching_executes_code_in_files_directory() { let temp_dir = Builder::new() .prefix("matching_executes_code_in_files_directory") @@ -85,6 +88,7 @@ fn matching_executes_code_in_files_directory() { } #[test] +#[serial(path_addicted)] fn matching_embedded_filename() { let temp_dir = Builder::new() .prefix("matching_embedded_filename") @@ -116,6 +120,7 @@ fn matching_embedded_filename() { } #[test] +#[serial(path_addicted)] /// Running "find . -execdir whatever \;" failed with a No such file or directory error. /// It's now fixed, and this is a regression test to check that it stays fixed. fn execdir_in_current_directory() { @@ -149,6 +154,7 @@ fn execdir_in_current_directory() { } #[test] +#[serial(path_addicted)] /// Regression test for "find / -execdir whatever \;" fn execdir_in_root_directory() { let temp_dir = Builder::new() @@ -188,6 +194,7 @@ fn execdir_in_root_directory() { } #[test] +#[serial(path_addicted)] fn matching_fails_if_executable_fails() { let temp_dir = Builder::new() .prefix("matching_fails_if_executable_fails") @@ -226,6 +233,7 @@ fn matching_fails_if_executable_fails() { } #[test] +#[serial(path_addicted)] fn matching_multi_executes_code() { let temp_dir = Builder::new() .prefix("matching_executes_code") @@ -259,6 +267,7 @@ fn matching_multi_executes_code() { } #[test] +#[serial(path_addicted)] fn execdir_multi_in_current_directory() { let temp_dir = Builder::new() .prefix("execdir_in_current_directory") @@ -293,6 +302,7 @@ fn execdir_multi_in_current_directory() { } #[test] +#[serial(path_addicted)] fn multi_set_exit_code_if_executable_fails() { let temp_dir = Builder::new() .prefix("multi_set_exit_code_if_executable_fails") @@ -327,6 +337,7 @@ fn multi_set_exit_code_if_executable_fails() { } #[test] +#[serial(path_addicted)] fn multi_set_exit_code_if_command_fails() { let abbbc = get_dir_entry_for("test_data/simple", "abbbc"); let matcher = MultiExecMatcher::new("1337", &["abc"], true).expect("Failed to create matcher"); @@ -336,3 +347,334 @@ fn multi_set_exit_code_if_command_fails() { matcher.finished_dir(Path::new("test_data/simple"), &mut matcher_io); assert!(matcher_io.exit_code() == 1); } + +// -- tests for checking path integrity +// SAFETY: +// The only use of unsafe was for the std::env::set_var function. +// Safety is guaranteed because all tests that directly depend +// on the PATH variable were marked with the macro #[serial(path_addicted)], +// making the execution of this serial and avoiding concurrency +// and data-race problems. Furthermore, any test that somehow +// changes the PATH, immediately after execution returns the +// PATH to the original value of the variable before the change. + +#[test] +#[serial(path_addicted)] +fn test_empty_path() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("1337", &["abc"], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn test_non_absolute_path() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}relative{}valid_segment4", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("1337", &["abc"], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn test_file_path() { + let temp_dir = tempdir().expect("Failed to create temp file"); + let temp_dir_path = temp_dir.path(); + let absolute_file_path = temp_dir_path.join("__test_temp_file"); + File::create(absolute_file_path.clone()) + .expect("Failed to create file") + .write_all(b"foo") + .expect("Failed to write to file"); + + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, + separator, + valid_segment2, + separator, + valid_segment3, + separator, + absolute_file_path.to_str().unwrap() + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("1337", &["abc"], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn valid_path_single_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let valid_path = format!( + "{}{}{}{}{}", + valid_segment1, separator, valid_segment2, separator, valid_segment3 + ); + + unsafe { + env::set_var("PATH", valid_path); + } + let matcher = SingleExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_ok()); +} + +#[test] +#[serial(path_addicted)] +fn valid_path_multi_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let valid_path = format!( + "{}{}{}{}{}", + valid_segment1, separator, valid_segment2, separator, valid_segment3 + ); + + unsafe { + env::set_var("PATH", valid_path); + } + let matcher = MultiExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_ok()); +} + +#[test] +#[serial(path_addicted)] +fn empty_path_segment_single_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = SingleExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn relative_path_segment_single_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}relative{}valid_segment4", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = SingleExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn file_path_segment_single_exec() { + let temp_dir = tempdir().expect("Failed to create temp file"); + let temp_dir_path = temp_dir.path(); + let absolute_file_path = temp_dir_path.join("test_file"); + + File::create(&absolute_file_path) + .expect("Failed to create file") + .write_all(b"foo") + .expect("Failed to write to file"); + + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, + separator, + valid_segment2, + separator, + valid_segment3, + separator, + absolute_file_path.to_str().unwrap() + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = SingleExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn empty_path_segment_multi_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn relative_path_segment_multi_exec() { + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}relative{}valid_segment4", + valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +#[test] +#[serial(path_addicted)] +fn file_path_segment_multi_exec() { + let temp_dir = tempdir().expect("Failed to create temp file"); + let temp_dir_path = temp_dir.path(); + let absolute_file_path = temp_dir_path.join("test_file"); + + File::create(&absolute_file_path) + .expect("Failed to create file") + .write_all(b"foo") + .expect("Failed to write to file"); + + let original_path = env::var("PATH").unwrap(); + let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); + let separator = get_path_separator(); + + let invalid_path = format!( + "{}{}{}{}{}{}{}", + valid_segment1, + separator, + valid_segment2, + separator, + valid_segment3, + separator, + absolute_file_path.to_str().unwrap() + ); + + unsafe { + env::set_var("PATH", invalid_path); + } + let matcher = MultiExecMatcher::new("true", &[], true); + unsafe { + env::set_var("PATH", original_path); + } + + assert!(matcher.is_err()); +} + +fn get_path_separator() -> char { + if cfg!(windows) { + ';' + } else { + ':' + } +} + +fn get_valid_segments() -> (String, String, String) { + if cfg!(windows) { + ( + "C:\\Windows".to_string(), + "C:\\Program Files".to_string(), + "C:\\Program Files (x86)".to_string(), + ) + } else { + ( + "/usr/bin".to_string(), + "/usr/sbin".to_string(), + "/usr/local/bin".to_string(), + ) + } +} From 9d67cfceaf3621e8b185acf515780438eaa0567a Mon Sep 17 00:00:00 2001 From: Sami Daniel Date: Tue, 10 Jun 2025 14:43:53 -0300 Subject: [PATCH 2/2] matches/exec: change tests and check_path implementation renamed: check_path_integrity -> check_path_entries_absolute The function check_path_entries_absolute now only looks for paths that are not absolute or that are empty. In addition, if possible, if there is any error related to the PATH, show the exact segment where the error occurred. tests: The tests that used to reside in exec_unit_tests.rs are now in exec.rs, directly handling a fake of the PATH for testing, eliminating the need to mark tests with #[serial] or use unsafe { env::set_var(...) }. Co-authored-by: Tavian --- src/find/matchers/exec.rs | 456 ++++++++++++++++++++++++++++++++++++-- tests/exec_unit_tests.rs | 346 +---------------------------- 2 files changed, 438 insertions(+), 364 deletions(-) mode change 100644 => 100755 src/find/matchers/exec.rs diff --git a/src/find/matchers/exec.rs b/src/find/matchers/exec.rs old mode 100644 new mode 100755 index 4741cf5d..6fec4b98 --- a/src/find/matchers/exec.rs +++ b/src/find/matchers/exec.rs @@ -14,32 +14,30 @@ use std::process::Command; use super::{Matcher, MatcherIO, WalkEntry}; -fn check_path_integrity() -> Result<(), Box> { - let path_dirs = env::var("PATH")?; - for dir_entry in env::split_paths(&path_dirs) { - // We can securely unwrap (or expect) the value of dir_entry string - // conversion on message error cause the env::var returns an VarError - // variant that indicates if the variable (in this case PATH) contains - // invalid Unicode data. - let dir_entry_str = dir_entry.to_str().expect("Unexpected conversion error"); - if !dir_entry.is_absolute() || dir_entry.is_file() || dir_entry_str.is_empty() { - return Err(format!( - "The PATH environment variable contains non-absolute paths, \ - files, or empty paths. Segment that caused the error: '{}'", - dir_entry_str - ) - .into()); +fn check_path_entries_absolute(path: Option) -> Result<(), Box> { + if let Some(path_dirs) = path { + for dir_entry in env::split_paths(&path_dirs) { + if !dir_entry.is_absolute() || dir_entry.as_os_str().is_empty() { + return Err(format!( + "The PATH environment variable contains non-absolute paths or empty paths. \ + Segment that caused the error: '{}'", + dir_entry.as_os_str().to_string_lossy() + ) + .into()); + } } } Ok(()) } +#[derive(Debug)] enum Arg { FileArg(Vec), LiteralArg(OsString), } +#[derive(Debug)] pub struct SingleExecMatcher { executable: String, args: Vec, @@ -51,9 +49,18 @@ impl SingleExecMatcher { executable: &str, args: &[&str], exec_in_parent_dir: bool, + ) -> Result> { + Self::new_with_path(executable, args, exec_in_parent_dir, env::var_os("PATH")) + } + + fn new_with_path( + executable: &str, + args: &[&str], + exec_in_parent_dir: bool, + path: Option, ) -> Result> { if exec_in_parent_dir { - check_path_integrity()?; + check_path_entries_absolute(path)?; } let transformed_args = args @@ -124,6 +131,7 @@ impl Matcher for SingleExecMatcher { } } +#[derive(Debug)] pub struct MultiExecMatcher { executable: String, args: Vec, @@ -137,9 +145,18 @@ impl MultiExecMatcher { executable: &str, args: &[&str], exec_in_parent_dir: bool, + ) -> Result> { + Self::new_with_path(executable, args, exec_in_parent_dir, env::var_os("PATH")) + } + + fn new_with_path( + executable: &str, + args: &[&str], + exec_in_parent_dir: bool, + path: Option, ) -> Result> { if exec_in_parent_dir { - check_path_integrity()?; + check_path_entries_absolute(path)?; } let transformed_args = args.iter().map(OsString::from).collect(); @@ -247,7 +264,406 @@ impl Matcher for MultiExecMatcher { } } +/// Only tests related to path checking here, because we need to call out to an external executable. +/// See `tests/exec_unit_tests.rs` instead. #[cfg(test)] -/// No tests here, because we need to call out to an external executable. See -/// `tests/exec_unit_tests.rs` instead. -mod tests {} +mod check_path_tests { + use super::*; + use std::path::MAIN_SEPARATOR; + + #[cfg(unix)] + use std::os::unix::ffi::OsStringExt; + #[cfg(unix)] + const PATH_SEPARATOR: char = ':'; + #[cfg(windows)] + const PATH_SEPARATOR: char = ';'; + #[cfg(windows)] + use std::os::windows::ffi::OsStringExt; + + // Helper to create platform-specific absolute paths + fn abs_path(component: &str) -> String { + format!("{}{}", MAIN_SEPARATOR, component) + } + + // Helper to create platform-specific relative paths + fn rel_path(component: &str) -> String { + format!(".{}{}", MAIN_SEPARATOR, component) + } + + mod single_exec_matcher_tests { + use super::*; + + #[test] + fn single_exec_matcher_valid_path() { + #[cfg(unix)] + let path = format!("{}{}{}", abs_path("usr"), PATH_SEPARATOR, abs_path("bin")); + #[cfg(windows)] + let path = format!( + "C:{}{}C:{}", + abs_path("usr"), + PATH_SEPARATOR, + abs_path("bin") + ); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_ok()); + } + + #[test] + fn single_exec_matcher_multiple_valid_paths() { + #[cfg(unix)] + let path = format!( + "{}{}{}{}{}", + abs_path("a"), + PATH_SEPARATOR, + abs_path("b"), + PATH_SEPARATOR, + abs_path("c") + ); + #[cfg(windows)] + let path = format!( + "C:{}{}C:{}{}C:{}", + abs_path("a"), + PATH_SEPARATOR, + abs_path("b"), + PATH_SEPARATOR, + abs_path("c") + ); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_ok()); + } + + #[test] + fn single_exec_matcher_relative_path() { + let path = format!( + "{}{}{}", + abs_path("usr"), + PATH_SEPARATOR, + rel_path("relative") + ); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn single_exec_matcher_empty_path() { + let path = format!("{}{}{}", abs_path("usr"), PATH_SEPARATOR, ""); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn single_exec_matcher_empty_string_path() { + let result = + SingleExecMatcher::new_with_path("echo", &["test"], true, Some(OsString::from(""))); + assert!(result.is_err()); + } + + #[test] + fn single_exec_matcher_valid_then_invalid_path() { + let path = format!( + "{}{}{}{}{}", + abs_path("valid1"), + PATH_SEPARATOR, + rel_path("invalid"), + PATH_SEPARATOR, + abs_path("valid2") + ); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn single_exec_matcher_undefined_path() { + let result = SingleExecMatcher::new_with_path("echo", &["test"], true, None); + assert!(result.is_ok()); + } + + #[test] + fn single_exec_matcher_no_validation_when_not_needed() { + let path = format!("{}{}{}", "relative", PATH_SEPARATOR, ""); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + false, + Some(OsString::from(path)), + ); + assert!(result.is_ok()); + } + + #[test] + fn single_exec_matcher_relative_path_error_message() { + let relative_component = rel_path("relative"); + #[cfg(unix)] + let path = format!( + "{}{}{}", + abs_path("usr"), + PATH_SEPARATOR, + relative_component + ); + #[cfg(windows)] + let path = format!( + "C:{}{}C:{}", + abs_path("usr"), + PATH_SEPARATOR, + relative_component + ); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + let err = result.unwrap_err(); + let err_msg = err.to_string(); + assert!( + err_msg.contains(&relative_component), + "Error message should contain relative path component" + ); + } + + #[test] + fn single_exec_matcher_empty_path_error_message() { + #[cfg(unix)] + let path = format!("{}{}{}", abs_path("usr"), PATH_SEPARATOR, ""); + #[cfg(windows)] + let path = format!("C:{}{}{}", abs_path("usr"), PATH_SEPARATOR, ""); + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + let err = result.unwrap_err(); + let err_msg = err.to_string(); + assert!( + err_msg.contains("''"), + "Error message should contain empty path indicator" + ); + } + + // Platform-specific non-UTF8 tests + #[cfg(unix)] + #[test] + #[ignore] + fn single_exec_matcher_does_not_reject_non_utf8_unix() { + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from_vec(vec![ + b'\x2F', b'\x00', b'\x75', b'\x00', b'\x73', b'\x00', b'\x72', b'\x00', + b'\x2F', b'\x00', b'\x62', b'\x00', b'\x69', b'\x00', b'\x6E', b'\x00', + b'\x3A', b'\x00', b'\x2F', b'\x00', b'\x62', b'\x00', b'\x69', b'\x00', + b'\x6E', b'\x00', + ])), + ); + assert!(result.is_ok()); + } + + #[cfg(windows)] + #[test] + #[ignore] + fn single_exec_matcher_does_not_reject_non_utf8_windows() { + let result = SingleExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from_wide(&[ + 0x0043, 0x003A, 0x005C, 0x0076, 0x0061, 0x006C, 0x0069, 0x0064, 0x003B, 0xD800, + 0xDC00, + ])), + ); + assert!(result.is_ok()); + } + } + + mod multi_exec_matcher_test { + use super::*; + + // Tests mirroring the single_exec tests with MultiExecMatcher + // (Same test structure as above but using MultiExecMatcher) + #[test] + fn multi_exec_matcher_valid_path() { + #[cfg(unix)] + let path = format!("{}{}{}", abs_path("usr"), PATH_SEPARATOR, abs_path("bin")); + #[cfg(windows)] + let path = format!( + "C:{}{}C:{}", + abs_path("usr"), + PATH_SEPARATOR, + abs_path("bin") + ); + + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_ok()); + } + + #[test] + fn multi_exec_matcher_multiple_valid_paths() { + #[cfg(unix)] + let path = format!( + "{}{}{}{}{}", + abs_path("a"), + PATH_SEPARATOR, + abs_path("b"), + PATH_SEPARATOR, + abs_path("c") + ); + #[cfg(windows)] + let path = format!( + "C:{}{}C:{}{}C:{}", + abs_path("a"), + PATH_SEPARATOR, + abs_path("b"), + PATH_SEPARATOR, + abs_path("c") + ); + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path.clone())), + ); + + assert!(result.is_ok()); + } + + #[test] + fn multi_exec_matcher_relative_path() { + let path = format!( + "{}{}{}", + abs_path("usr"), + PATH_SEPARATOR, + rel_path("relative") + ); + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn multi_exec_matcher_empty_path() { + let path = format!("{}{}{}", abs_path("usr"), PATH_SEPARATOR, ""); + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn multi_exec_matcher_empty_string_path() { + let result = + MultiExecMatcher::new_with_path("echo", &["test"], true, Some(OsString::from(""))); + assert!(result.is_err()); + } + + #[test] + fn multi_exec_matcher_valid_then_invalid_path() { + let path = format!( + "{}{}{}{}{}", + abs_path("valid1"), + PATH_SEPARATOR, + rel_path("invalid"), + PATH_SEPARATOR, + abs_path("valid2") + ); + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from(path)), + ); + assert!(result.is_err()); + } + + #[test] + fn multi_exec_matcher_undefined_path() { + let result = MultiExecMatcher::new_with_path("echo", &["test"], true, None); + assert!(result.is_ok()); + } + + #[test] + fn multi_exec_matcher_no_validation_when_not_needed() { + let path = format!("{}{}{}", "relative", PATH_SEPARATOR, ""); + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + false, + Some(OsString::from(path)), + ); + assert!(result.is_ok()); + } + + // Platform-specific non-UTF8 tests + #[cfg(unix)] + #[test] + #[ignore] + fn multi_exec_matcher_does_not_reject_non_utf8_unix() { + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from_vec(vec![ + b'\x2F', b'\x00', b'\x75', b'\x00', b'\x73', b'\x00', b'\x72', b'\x00', + b'\x2F', b'\x00', b'\x62', b'\x00', b'\x69', b'\x00', b'\x6E', b'\x00', + b'\x3A', b'\x00', b'\x2F', b'\x00', b'\x62', b'\x00', b'\x69', b'\x00', + b'\x6E', b'\x00', + ])), + ); + assert!(result.is_ok()); + } + + #[cfg(windows)] + #[test] + #[ignore] + fn multi_exec_matcher_does_not_reject_non_utf8_windows() { + let result = MultiExecMatcher::new_with_path( + "echo", + &["test"], + true, + Some(OsString::from_wide(&[ + 0x0043, 0x003A, 0x005C, 0x0076, 0x0061, 0x006C, 0x0069, 0x0064, 0x003B, 0xD800, + 0xDC00, + ])), + ); + assert!(result.is_ok()); + } + } +} diff --git a/tests/exec_unit_tests.rs b/tests/exec_unit_tests.rs index 3285253b..e952e405 100644 --- a/tests/exec_unit_tests.rs +++ b/tests/exec_unit_tests.rs @@ -8,12 +8,11 @@ /// ! But as the tests require running an external executable, they need to be run /// ! as integration tests so we can ensure that our testing-commandline binary /// ! has been built. -use serial_test::serial; use std::env; use std::fs::File; -use std::io::{Read, Write}; +use std::io::Read; use std::path::Path; -use tempfile::{tempdir, Builder}; +use tempfile::Builder; use common::test_helpers::{ fix_up_slashes, get_dir_entry_for, path_to_testing_commandline, FakeDependencies, @@ -24,7 +23,6 @@ use findutils::find::matchers::{Matcher, MatcherIO}; mod common; #[test] -#[serial(path_addicted)] fn matching_executes_code() { let temp_dir = Builder::new() .prefix("matching_executes_code") @@ -56,7 +54,6 @@ fn matching_executes_code() { } #[test] -#[serial(path_addicted)] fn matching_executes_code_in_files_directory() { let temp_dir = Builder::new() .prefix("matching_executes_code_in_files_directory") @@ -88,7 +85,6 @@ fn matching_executes_code_in_files_directory() { } #[test] -#[serial(path_addicted)] fn matching_embedded_filename() { let temp_dir = Builder::new() .prefix("matching_embedded_filename") @@ -120,7 +116,6 @@ fn matching_embedded_filename() { } #[test] -#[serial(path_addicted)] /// Running "find . -execdir whatever \;" failed with a No such file or directory error. /// It's now fixed, and this is a regression test to check that it stays fixed. fn execdir_in_current_directory() { @@ -154,7 +149,6 @@ fn execdir_in_current_directory() { } #[test] -#[serial(path_addicted)] /// Regression test for "find / -execdir whatever \;" fn execdir_in_root_directory() { let temp_dir = Builder::new() @@ -194,7 +188,6 @@ fn execdir_in_root_directory() { } #[test] -#[serial(path_addicted)] fn matching_fails_if_executable_fails() { let temp_dir = Builder::new() .prefix("matching_fails_if_executable_fails") @@ -233,7 +226,6 @@ fn matching_fails_if_executable_fails() { } #[test] -#[serial(path_addicted)] fn matching_multi_executes_code() { let temp_dir = Builder::new() .prefix("matching_executes_code") @@ -267,7 +259,6 @@ fn matching_multi_executes_code() { } #[test] -#[serial(path_addicted)] fn execdir_multi_in_current_directory() { let temp_dir = Builder::new() .prefix("execdir_in_current_directory") @@ -302,7 +293,6 @@ fn execdir_multi_in_current_directory() { } #[test] -#[serial(path_addicted)] fn multi_set_exit_code_if_executable_fails() { let temp_dir = Builder::new() .prefix("multi_set_exit_code_if_executable_fails") @@ -337,7 +327,6 @@ fn multi_set_exit_code_if_executable_fails() { } #[test] -#[serial(path_addicted)] fn multi_set_exit_code_if_command_fails() { let abbbc = get_dir_entry_for("test_data/simple", "abbbc"); let matcher = MultiExecMatcher::new("1337", &["abc"], true).expect("Failed to create matcher"); @@ -347,334 +336,3 @@ fn multi_set_exit_code_if_command_fails() { matcher.finished_dir(Path::new("test_data/simple"), &mut matcher_io); assert!(matcher_io.exit_code() == 1); } - -// -- tests for checking path integrity -// SAFETY: -// The only use of unsafe was for the std::env::set_var function. -// Safety is guaranteed because all tests that directly depend -// on the PATH variable were marked with the macro #[serial(path_addicted)], -// making the execution of this serial and avoiding concurrency -// and data-race problems. Furthermore, any test that somehow -// changes the PATH, immediately after execution returns the -// PATH to the original value of the variable before the change. - -#[test] -#[serial(path_addicted)] -fn test_empty_path() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("1337", &["abc"], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn test_non_absolute_path() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}relative{}valid_segment4", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("1337", &["abc"], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn test_file_path() { - let temp_dir = tempdir().expect("Failed to create temp file"); - let temp_dir_path = temp_dir.path(); - let absolute_file_path = temp_dir_path.join("__test_temp_file"); - File::create(absolute_file_path.clone()) - .expect("Failed to create file") - .write_all(b"foo") - .expect("Failed to write to file"); - - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, - separator, - valid_segment2, - separator, - valid_segment3, - separator, - absolute_file_path.to_str().unwrap() - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("1337", &["abc"], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn valid_path_single_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let valid_path = format!( - "{}{}{}{}{}", - valid_segment1, separator, valid_segment2, separator, valid_segment3 - ); - - unsafe { - env::set_var("PATH", valid_path); - } - let matcher = SingleExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_ok()); -} - -#[test] -#[serial(path_addicted)] -fn valid_path_multi_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let valid_path = format!( - "{}{}{}{}{}", - valid_segment1, separator, valid_segment2, separator, valid_segment3 - ); - - unsafe { - env::set_var("PATH", valid_path); - } - let matcher = MultiExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_ok()); -} - -#[test] -#[serial(path_addicted)] -fn empty_path_segment_single_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = SingleExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn relative_path_segment_single_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}relative{}valid_segment4", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = SingleExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn file_path_segment_single_exec() { - let temp_dir = tempdir().expect("Failed to create temp file"); - let temp_dir_path = temp_dir.path(); - let absolute_file_path = temp_dir_path.join("test_file"); - - File::create(&absolute_file_path) - .expect("Failed to create file") - .write_all(b"foo") - .expect("Failed to write to file"); - - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, - separator, - valid_segment2, - separator, - valid_segment3, - separator, - absolute_file_path.to_str().unwrap() - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = SingleExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn empty_path_segment_multi_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn relative_path_segment_multi_exec() { - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}relative{}valid_segment4", - valid_segment1, separator, valid_segment2, separator, valid_segment3, separator, separator - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -#[test] -#[serial(path_addicted)] -fn file_path_segment_multi_exec() { - let temp_dir = tempdir().expect("Failed to create temp file"); - let temp_dir_path = temp_dir.path(); - let absolute_file_path = temp_dir_path.join("test_file"); - - File::create(&absolute_file_path) - .expect("Failed to create file") - .write_all(b"foo") - .expect("Failed to write to file"); - - let original_path = env::var("PATH").unwrap(); - let (valid_segment1, valid_segment2, valid_segment3) = get_valid_segments(); - let separator = get_path_separator(); - - let invalid_path = format!( - "{}{}{}{}{}{}{}", - valid_segment1, - separator, - valid_segment2, - separator, - valid_segment3, - separator, - absolute_file_path.to_str().unwrap() - ); - - unsafe { - env::set_var("PATH", invalid_path); - } - let matcher = MultiExecMatcher::new("true", &[], true); - unsafe { - env::set_var("PATH", original_path); - } - - assert!(matcher.is_err()); -} - -fn get_path_separator() -> char { - if cfg!(windows) { - ';' - } else { - ':' - } -} - -fn get_valid_segments() -> (String, String, String) { - if cfg!(windows) { - ( - "C:\\Windows".to_string(), - "C:\\Program Files".to_string(), - "C:\\Program Files (x86)".to_string(), - ) - } else { - ( - "/usr/bin".to_string(), - "/usr/sbin".to_string(), - "/usr/local/bin".to_string(), - ) - } -}