Merge pull request #8947 from naoNao89/fix/mkdir-stack-overflow

mkdir: Fix stack overflow with deeply nested directories

src/uu/mkdir/src/mkdir.rs

@@ -216,8 +216,8 @@ fn chmod(_path: &Path, _mode: u32) -> UResult<()> {
     Ok(())
 }
 
-// Return true if the directory at `path` has been created by this call.
-// `is_parent` argument is not used on windows
+// Create a directory at the given path.
+// Uses iterative approach instead of recursion to avoid stack overflow with deep nesting.
 #[allow(unused_variables)]
 fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
     let path_exists = path.exists();
@@ -231,15 +231,41 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
         return Ok(());
     }
 
+    // Iterative implementation: collect all directories to create, then create them
+    // This avoids stack overflow with deeply nested directories
     if config.recursive {
-        match path.parent() {
-            Some(p) => create_dir(p, true, config)?,
-            None => {
-                USimpleError::new(1, translate!("mkdir-error-failed-to-create-tree"));
+        // Pre-allocate approximate capacity to avoid reallocations
+        let mut dirs_to_create = Vec::with_capacity(16);
+        let mut current = path;
+
+        // First pass: collect all parent directories
+        while let Some(parent) = current.parent() {
+            if parent == Path::new("") {
+                break;
+            }
+            dirs_to_create.push(parent);
+            current = parent;
+        }
+
+        // Second pass: create directories from root to leaf
+        // Only create those that don't exist
+        for dir in dirs_to_create.iter().rev() {
+            if !dir.exists() {
+                create_single_dir(dir, true, config)?;
             }
         }
     }
 
+    // Create the target directory
+    create_single_dir(path, is_parent, config)
+}
+
+// Helper function to create a single directory with appropriate permissions
+// `is_parent` argument is not used on windows
+#[allow(unused_variables)]
+fn create_single_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
+    let path_exists = path.exists();
+
     match std::fs::create_dir(path) {
         Ok(()) => {
             if config.verbose {
@@ -287,7 +313,24 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
             Ok(())
         }
 
-        Err(_) if path.is_dir() => Ok(()),
+        Err(_) if path.is_dir() => {
+            // Directory already exists - check if this is a logical directory creation
+            // (i.e., not just a parent reference like "test_dir/..")
+            let ends_with_parent_dir = matches!(
+                path.components().next_back(),
+                Some(std::path::Component::ParentDir)
+            );
+
+            // Print verbose message for logical directories, even if they exist
+            // This matches GNU behavior for paths like "test_dir/../test_dir_a"
+            if config.verbose && is_parent && config.recursive && !ends_with_parent_dir {
+                println!(
+                    "{}",
+                    translate!("mkdir-verbose-created-directory", "util_name" => uucore::util_name(), "path" => path.quote())
+                );
+            }
+            Ok(())
+        }
         Err(e) => Err(e.into()),
     }
 }

tests/by-util/test_mkdir.rs

@@ -3,7 +3,7 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-// spell-checker:ignore bindgen testtest
+// spell-checker:ignore bindgen testtest casetest CASETEST
 
 #![allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)]
 
@@ -398,6 +398,31 @@ fn test_mkdir_trailing_dot_and_slash() {
     println!("ls dest {}", result.stdout_str());
 }
 
+#[test]
+fn test_mkdir_trailing_spaces_and_dots() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Test trailing space
+    scene.ucmd().arg("-p").arg("test ").succeeds();
+    assert!(at.dir_exists("test "));
+
+    // Test multiple trailing spaces
+    scene.ucmd().arg("-p").arg("test   ").succeeds();
+    assert!(at.dir_exists("test   "));
+
+    // Test leading dot (hidden on Unix)
+    scene.ucmd().arg("-p").arg(".hidden").succeeds();
+    assert!(at.dir_exists(".hidden"));
+
+    // Test trailing dot (should work)
+    scene.ucmd().arg("-p").arg("test.").succeeds();
+    assert!(at.dir_exists("test."));
+
+    // Test multiple leading dots
+    scene.ucmd().arg("-p").arg("...test").succeeds();
+    assert!(at.dir_exists("...test"));
+}
 #[test]
 #[cfg(not(windows))]
 fn test_umask_compliance() {
@@ -466,3 +491,356 @@ fn test_selinux_invalid() {
     // invalid context, so, no directory
     assert!(!at.dir_exists(dest));
 }
+
+#[test]
+fn test_mkdir_deep_nesting() {
+    // Regression test for stack overflow with deeply nested directories.
+    // The iterative implementation should handle arbitrary depth without stack overflow.
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Create a path with 350 levels of nesting
+    let depth = 350;
+    let dir_name = "d";
+    let mut path = std::path::PathBuf::new();
+    for _ in 0..depth {
+        path.push(dir_name);
+    }
+
+    scene.ucmd().arg("-p").arg(&path).succeeds();
+
+    assert!(at.dir_exists(&path));
+}
+
+#[test]
+fn test_mkdir_dot_components() {
+    // Test handling of "." (current directory) components
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Test case from review comment: test/././test2/././test3/././test4
+    // GNU mkdir normalizes this path and creates test/test2/test3/test4
+    let path = "test/././test2/././test3/././test4";
+    scene.ucmd().arg("-p").arg(path).succeeds();
+
+    // Verify expected structure exists (GNU compatibility)
+    assert!(at.dir_exists("test/test2/test3/test4"));
+
+    // Test leading "." - should create test_dot/test_dot2
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("./test_dot/test_dot2")
+        .succeeds();
+    assert!(at.dir_exists("test_dot/test_dot2"));
+
+    // Test mixed "." and normal components
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("mixed/./normal/./path")
+        .succeeds();
+    assert!(at.dir_exists("mixed/normal/path"));
+
+    // Test that the command works without creating redundant directories
+    // The key test is that it doesn't fail or create incorrect structure
+    // The actual filesystem behavior (whether literal "." dirs exist)
+    // may vary, but the logical result should be correct
+}
+
+#[test]
+fn test_mkdir_parent_dir_components() {
+    // Test handling of ".." (parent directory) components
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("base");
+    at.mkdir("base/child");
+
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("base/child/../sibling")
+        .succeeds();
+    assert!(at.dir_exists("base/sibling"));
+
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("base/child/../../other")
+        .succeeds();
+    assert!(at.dir_exists("other"));
+
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("base/child/../sibling")
+        .succeeds();
+    assert!(at.dir_exists("base/sibling"));
+}
+
+#[test]
+fn test_mkdir_mixed_special_components() {
+    // Test complex paths with both "." and ".." components
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("./start/./middle/../end/./final")
+        .succeeds();
+    assert!(at.dir_exists("start/end/final"));
+}
+
+#[test]
+fn test_mkdir_control_characters() {
+    // Test handling of control characters in filenames
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Test NUL byte (should fail on most systems)
+    // Note: This test is skipped because NUL bytes cause panics in the test framework
+    // let result = scene.ucmd().arg("test\x00name").run();
+    // assert!(!result.succeeded());
+
+    // Test newline character (may fail on Windows)
+    let result = scene.ucmd().arg("-p").arg("test\nname").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("test\nname"));
+    }
+
+    // Test tab character (may fail on Windows)
+    let result = scene.ucmd().arg("-p").arg("test\tname").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("test\tname"));
+    }
+
+    // Test space character in directory name (should work on all systems)
+    scene.ucmd().arg("-p").arg("test name").succeeds();
+    assert!(at.dir_exists("test name"));
+
+    // Test double quotes in path - platform-specific behavior expected
+    // On Linux/Unix: Should succeed and create directory with quotes
+    // On Windows: Should fail with "Invalid argument" error
+    #[cfg(unix)]
+    {
+        scene.ucmd().arg("-pv").arg("a/\"\"/b/c").succeeds();
+        assert!(at.dir_exists("a/\"\"/b/c"));
+    }
+    #[cfg(windows)]
+    {
+        let result = scene.ucmd().arg("-pv").arg("a/\"\"/b/c").run();
+        // Should fail after creating 'a' directory
+        assert!(at.dir_exists("a"));
+        assert!(!at.dir_exists("a/\"\""));
+        assert!(!result.succeeded());
+        // Windows should reject directory names with quotes
+        // We don't assert on specific error message as it varies by Windows version
+    }
+
+    // Test single quotes in path - should work on both Unix and Windows
+    scene.ucmd().arg("-p").arg("a/''/b/c").succeeds();
+    assert!(at.dir_exists("a/''/b/c"));
+}
+
+#[test]
+fn test_mkdir_maximum_path_length() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Test moderate length path (should work on all systems)
+    let long_path = "a".repeat(50) + "/" + &"b".repeat(50) + "/" + &"c".repeat(50);
+    scene.ucmd().arg("-p").arg(&long_path).succeeds();
+    assert!(at.dir_exists(&long_path));
+
+    // Test longer but reasonable path
+    let longer_path = "x".repeat(100) + "/" + &"y".repeat(50) + "/" + &"z".repeat(30);
+    scene.ucmd().arg("-p").arg(&longer_path).succeeds();
+    assert!(at.dir_exists(&longer_path));
+
+    // Test extremely long path (may fail on some systems)
+    let very_long_path = "very_long_directory_name_".repeat(20) + "/final";
+    let result = scene.ucmd().arg("-p").arg(&very_long_path).run();
+    if result.succeeded() {
+        assert!(at.dir_exists(&very_long_path));
+    }
+    // If it fails, that's acceptable on some systems with path limits
+}
+
+#[test]
+fn test_mkdir_reserved_device_names() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // These should work on Unix but may fail on Windows
+    let result = scene.ucmd().arg("-p").arg("CON").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("CON"));
+    }
+
+    let result = scene.ucmd().arg("-p").arg("PRN").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("PRN"));
+    }
+
+    let result = scene.ucmd().arg("-p").arg("AUX").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("AUX"));
+    }
+
+    // Test device names with extensions
+    let result = scene.ucmd().arg("-p").arg("COM1").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("COM1"));
+    }
+
+    let result = scene.ucmd().arg("-p").arg("LPT1").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("LPT1"));
+    }
+}
+
+#[test]
+fn test_mkdir_case_sensitivity() {
+    // Test case sensitivity behavior (varies by filesystem)
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Create directory with lowercase name
+    scene.ucmd().arg("-p").arg("CaseTest").succeeds();
+    assert!(at.dir_exists("CaseTest"));
+
+    // Try to create directory with different case
+    // On case-sensitive filesystems: creates separate directory
+    // On case-insensitive filesystems: fails (directory already exists)
+    let result = scene.ucmd().arg("-p").arg("casetest").run();
+
+    // The test passes regardless of filesystem behavior
+    // We just verify the command doesn't crash
+    if result.succeeded() {
+        // Case-sensitive filesystem - both exist
+        assert!(at.dir_exists("CaseTest"));
+        assert!(at.dir_exists("casetest"));
+    } else {
+        // Case-insensitive filesystem - only one exists
+        assert!(at.dir_exists("CaseTest"));
+    }
+
+    // Test mixed case variations
+    scene.ucmd().arg("-p").arg("CASETEST").succeeds();
+    scene.ucmd().arg("-p").arg("caseTEST").succeeds();
+}
+
+#[test]
+fn test_mkdir_network_paths() {
+    // Test network path formats (UNC paths on Windows)
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Test UNC-style path prefix (may fail on some systems)
+    let result = scene.ucmd().arg("-p").arg("//server/share/test").run();
+    if result.succeeded() {
+        assert!(at.dir_exists("//server/share/test"));
+    }
+    // If it fails, that's acceptable on some systems with read-only restrictions
+
+    // Test path that looks like network but is actually local
+    scene.ucmd().arg("-p").arg("server_share_test").succeeds();
+    assert!(at.dir_exists("server_share_test"));
+
+    // Test path with double slash in middle (should work)
+    scene.ucmd().arg("-p").arg("test//double//slash").succeeds();
+    assert!(at.dir_exists("test//double//slash"));
+}
+
+#[test]
+fn test_mkdir_environment_expansion() {
+    // Test that mkdir doesn't expand environment variables (unlike some shells)
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    unsafe {
+        std::env::set_var("TEST_VAR", "expanded_value");
+    }
+
+    // Create directory with literal $VAR (should not expand)
+    scene.ucmd().arg("-p").arg("$TEST_VAR/dir").succeeds();
+    assert!(at.dir_exists("$TEST_VAR/dir"));
+
+    // Verify the literal name exists, not the expanded value
+    assert!(!at.dir_exists("expanded_value/dir"));
+
+    // Test with braces
+    scene
+        .ucmd()
+        .arg("-p")
+        .arg("${TEST_VAR}_braced/dir")
+        .succeeds();
+    assert!(at.dir_exists("${TEST_VAR}_braced/dir"));
+
+    // Test with tilde (should not expand to home directory)
+    scene.ucmd().arg("-p").arg("~/test_dir").succeeds();
+    assert!(at.dir_exists("~/test_dir"));
+
+    unsafe {
+        std::env::remove_var("TEST_VAR");
+    }
+}
+
+#[test]
+fn test_mkdir_concurrent_creation() {
+    // Test concurrent mkdir -p operations: 10 iterations, 8 threads, 40 levels nesting
+    use std::thread;
+
+    for _ in 0..10 {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+
+        let mut dir = at.plus("concurrent_test");
+        dir.push("a");
+
+        for _ in 0..40 {
+            dir.push("a");
+        }
+
+        let path_str = dir.to_string_lossy().to_string();
+        let bin_path = scene.bin_path.clone();
+
+        let mut handles = vec![];
+
+        for _ in 0..8 {
+            let path_clone = path_str.clone();
+            let bin_path_clone = bin_path.clone();
+
+            let handle = thread::spawn(move || {
+                // Use the actual uutils mkdir binary to test the real implementation
+                let result = std::process::Command::new(&bin_path_clone)
+                    .arg("mkdir")
+                    .arg("-p")
+                    .arg(&path_clone)
+                    .current_dir(std::env::current_dir().unwrap())
+                    .output();
+
+                match result {
+                    Ok(output) => {
+                        assert!(
+                            output.status.success(),
+                            "mkdir failed: {}",
+                            String::from_utf8_lossy(&output.stderr)
+                        );
+                    }
+                    Err(e) => panic!("Failed to execute mkdir: {e}"),
+                }
+            });
+            handles.push(handle);
+        }
+
+        handles
+            .drain(..)
+            .map(|handle| handle.join().unwrap())
+            .count();
+
+        assert!(at.dir_exists(&path_str));
+    }
+}