From 17e3f0d20d2ceb33b9cda18a182cbbe65f64bed8 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Sun, 20 Sep 2026 23:06:54 -0700 Subject: [PATCH 1/7] install: name the directory component that could not be created --- src/uu/install/locales/en-US.ftl | 2 +- src/uu/install/locales/fr-FR.ftl | 2 +- src/uu/install/src/install.rs | 50 ++--- src/uucore/src/lib/features/safe_traversal.rs | 203 ++++++++++++------ tests/by-util/test_install.rs | 62 +++++- 5 files changed, 224 insertions(+), 95 deletions(-) diff --git a/src/uu/install/locales/en-US.ftl b/src/uu/install/locales/en-US.ftl index fdc1726c442..5c1c3ea1f29 100644 --- a/src/uu/install/locales/en-US.ftl +++ b/src/uu/install/locales/en-US.ftl @@ -23,7 +23,7 @@ install-help-unprivileged = do not require elevated privileges to change the own # Error messages install-error-dir-needs-arg = { $util_name } with -d requires at least one argument. -install-error-create-dir-failed = cannot create directory { $path } +install-error-create-dir-failed = cannot create directory { $path }: { $error } install-error-chmod-failed = failed to chmod { $path } install-error-chmod-failed-detailed = { $path }: chmod failed with error { $error } install-error-chown-failed = failed to chown { $path }: { $error } diff --git a/src/uu/install/locales/fr-FR.ftl b/src/uu/install/locales/fr-FR.ftl index 6423148c95d..e6651a06ba3 100644 --- a/src/uu/install/locales/fr-FR.ftl +++ b/src/uu/install/locales/fr-FR.ftl @@ -23,7 +23,7 @@ install-help-unprivileged = ne pas nécessiter de privilèges élevés pour chan # Messages d'erreur install-error-dir-needs-arg = { $util_name } avec -d nécessite au moins un argument. -install-error-create-dir-failed = échec de la création de { $path } +install-error-create-dir-failed = échec de la création de { $path } : { $error } install-error-chmod-failed = échec du chmod { $path } install-error-chmod-failed-detailed = { $path } : échec du chmod avec l'erreur { $error } install-error-chown-failed = échec du chown { $path } : { $error } diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 8a2e9957265..1b389f4a28a 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -72,7 +72,7 @@ enum InstallError { #[error("{}", translate!("install-error-dir-needs-arg", "util_name" => "install"))] DirNeedsArg, - #[error("{}", translate!("install-error-create-dir-failed", "path" => .0.quote()))] + #[error("{}", translate!("install-error-create-dir-failed", "path" => .0.quote(), "error" => strip_errno(.1)))] CreateDirFailed(PathBuf, #[source] std::io::Error), #[error("{}", translate!("install-error-chmod-failed", "path" => .0.quote()))] @@ -506,11 +506,21 @@ fn directory(paths: &[OsString], b: &Behavior) -> UResult<()> { // // NOTE: the GNU "install" sets the expected mode only for the // target directory. All created ancestor directories will have - // the default mode. Hence it is safe to use fs::create_dir_all - // and then only modify the target's dir mode. - if let Err(e) = fs::create_dir_all(&path_to_create).map_err_context( - || translate!("install-error-create-dir-failed", "path" => path_to_create.quote()), - ) { + // the default mode. Hence it is safe to create the whole chain + // with the default mode and then only modify the target's dir mode. + #[cfg(unix)] + { + // 0o777 is what `fs::create_dir_all` requests; the kernel + // applies the umask on top of it. + if let Err(e) = create_dir_all_safe(&path_to_create, 0o777) { + show!(InstallError::CreateDirFailed(e.path, e.error)); + continue; + } + } + #[cfg(not(unix))] + if let Err(e) = fs::create_dir_all(&path_to_create) + .map_err(|e| InstallError::CreateDirFailed(path_to_create.to_path_buf(), e)) + { show!(e); continue; } @@ -658,11 +668,6 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { None }; - // If -t is used, check if target exists as a file before trying to create directories - if b.target_dir.is_some() && target.exists() && !target.is_dir() { - return Err(InstallError::NotADirectory(target).into()); - } - if let Some(to_create) = to_create { let to_create_original = to_create; let to_create_owned; @@ -679,6 +684,14 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { _ => to_create, }; + // With -t, GNU reports a target that exists but is not a directory + // as a failure to access it, rather than a failed creation. The + // check runs on the trimmed path so that a trailing slash, which + // makes `exists()` fail with ENOTDIR, is handled the same way. + if b.target_dir.is_some() && to_create.exists() && !to_create.is_dir() { + return Err(InstallError::NotADirectory(to_create_original.to_path_buf()).into()); + } + let dir_exists = to_create.exists() && metadata(to_create).is_ok_and(|m| m.is_dir()); if dir_exists { @@ -735,20 +748,7 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { } } Err(e) => { - if e.kind() == std::io::ErrorKind::AlreadyExists - && to_create.exists() - && !to_create.is_dir() - { - return Err(InstallError::NotADirectory( - to_create_original.to_path_buf(), - ) - .into()); - } - return Err(InstallError::CreateDirFailed( - to_create_original.to_path_buf(), - e, - ) - .into()); + return Err(InstallError::CreateDirFailed(e.path, e.error).into()); } } } diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 587fabf36c7..25eb0927e9b 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -131,6 +131,7 @@ fn read_dir_entries(fd: &OwnedFd) -> io::Result> { } /// A directory file descriptor that enables safe traversal +#[derive(Debug)] pub struct DirFd { fd: OwnedFd, } @@ -510,66 +511,44 @@ impl DirFd { /// Returns the existing ancestor path and a list of components that need to be created. /// Uses `metadata` (follows symlinks) so that symlinks to directories are treated as /// existing ancestors rather than components to create. -fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec)> { +/// +/// Anything that is not a usable directory - a file, a dangling symlink, an +/// unreadable parent - ends up in the component list, so that the fd-based +/// descent in `create_dir_all_safe` reports the exact component that fails +/// instead of failing here with the whole path. +fn find_existing_ancestor(path: &Path) -> (PathBuf, Vec) { let mut current = path.to_path_buf(); let mut components: Vec = Vec::new(); loop { // Use metadata (follow symlinks) so that symlinks to directories are // treated as existing ancestors rather than components to create. - match fs::metadata(¤t) { - Ok(meta) => { - if meta.is_dir() { - // Found a directory (real or via symlink) - components.reverse(); - return Ok((current, components)); - } - // It's a file or other non-directory - treat as needing creation - if let Some(file_name) = current.file_name() { - components.push(file_name.to_os_string()); - } - if let Some(parent) = current.parent() { - if parent.as_os_str().is_empty() { - // Reached empty parent (for relative paths), use "." - components.reverse(); - return Ok((PathBuf::from("."), components)); - } - current = parent.to_path_buf(); - } else { - // Reached filesystem root - let root = if path.is_absolute() { - PathBuf::from("/") - } else { - PathBuf::from(".") - }; - components.reverse(); - return Ok((root, components)); - } + if fs::metadata(¤t).is_ok_and(|meta| meta.is_dir()) { + // Found a directory (real or via symlink) + components.reverse(); + return (current, components); + } + + if let Some(file_name) = current.file_name() { + components.push(file_name.to_os_string()); + } + match current.parent() { + // Reached an empty parent (for relative paths), use "." + Some(parent) if parent.as_os_str().is_empty() => { + components.reverse(); + return (PathBuf::from("."), components); } - Err(e) if e.kind() == io::ErrorKind::NotFound => { - // Doesn't exist, record component and move up to parent - if let Some(file_name) = current.file_name() { - components.push(file_name.to_os_string()); - } - if let Some(parent) = current.parent() { - if parent.as_os_str().is_empty() { - // Reached empty parent (for relative paths), use "." - components.reverse(); - return Ok((PathBuf::from("."), components)); - } - current = parent.to_path_buf(); + Some(parent) => current = parent.to_path_buf(), + // Reached the filesystem root + None => { + let root = if path.is_absolute() { + PathBuf::from("/") } else { - // Reached filesystem root - let root = if path.is_absolute() { - PathBuf::from("/") - } else { - PathBuf::from(".") - }; - components.reverse(); - return Ok((root, components)); - } + PathBuf::from(".") + }; + components.reverse(); + return (root, components); } - Err(e) => return Err(e), } } } @@ -580,6 +559,12 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec)> { /// path component. If a symlink to a directory exists, it is followed (GNU /// coreutils behavior). Dangling symlinks and non-directory entries are errors. /// +/// The reported errno matches what GNU utilities print for the same situation. +/// GNU calls `mkdir` first and only then descends into the component, so a name +/// that exists but cannot be descended into is reported as `EEXIST` (from the +/// `mkdir`) when it does not resolve at all, and as `ENOTDIR` when it resolves +/// to a non-directory. +/// /// # Arguments /// * `parent_fd` - The parent directory file descriptor /// * `name` - The name of the subdirectory to open or create @@ -597,15 +582,21 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu // Follow symlinks to directories (GNU coreutils behavior). // O_DIRECTORY in open_subdir ensures we only succeed if the // symlink resolves to a directory; dangling or non-dir symlinks error out. - parent_fd.open_subdir(name, SymlinkBehavior::Follow) + parent_fd + .open_subdir(name, SymlinkBehavior::Follow) + .map_err(|e| { + if e.kind() == io::ErrorKind::NotFound { + // A dangling symlink: the name itself exists, so + // `mkdir` would fail with EEXIST rather than ENOENT. + io::Error::from_raw_os_error(libc::EEXIST) + } else { + e + } + }) } - _ => Err(io::Error::new( - io::ErrorKind::AlreadyExists, - format!( - "path component exists but is not a directory: {}", - name.display() - ), - )), + // The name exists and is not a directory, so descending into it + // fails with ENOTDIR. + _ => Err(io::Error::from_raw_os_error(libc::ENOTDIR)), } } Err(e) if e.kind() == io::ErrorKind::NotFound => match parent_fd.mkdir_at(name, mode) { @@ -647,19 +638,72 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu /// /// # Returns /// A DirFd for the final created directory, or the first existing parent if -/// all directories already exist. +/// all directories already exist. On failure, the returned [`CreateDirError`] +/// names the prefix of `path` that could not be created, which is what GNU +/// utilities report. #[cfg(unix)] -pub fn create_dir_all_safe(path: &Path, mode: u32) -> io::Result { - let (existing_ancestor, components_to_create) = find_existing_ancestor(path)?; - let mut dir_fd = DirFd::open(&existing_ancestor, SymlinkBehavior::Follow)?; - - for component in &components_to_create { - dir_fd = open_or_create_subdir(&dir_fd, component.as_os_str(), mode)?; +pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result { + let (existing_ancestor, components_to_create) = find_existing_ancestor(path); + let mut dir_fd = DirFd::open(&existing_ancestor, SymlinkBehavior::Follow).map_err(|error| { + CreateDirError { + path: existing_ancestor.clone(), + error, + } + })?; + + for (index, component) in components_to_create.iter().enumerate() { + dir_fd = open_or_create_subdir(&dir_fd, component.as_os_str(), mode).map_err(|error| { + // `components_to_create` is the tail of `path`, so dropping the + // components that were not reached yields the failing prefix. + let mut failed = path.to_path_buf(); + for _ in index + 1..components_to_create.len() { + failed.pop(); + } + CreateDirError { + path: failed, + error, + } + })?; } Ok(dir_fd) } +/// Failure of [`create_dir_all_safe`], naming the path component that failed. +/// +/// GNU utilities report the first path prefix they could not create, not the +/// whole path that was requested, e.g. `install -d a/b/c` with a dangling +/// symlink `a` reports `a`. +#[cfg(unix)] +#[derive(Debug)] +pub struct CreateDirError { + /// The prefix of the requested path whose creation failed. + pub path: PathBuf, + /// The underlying OS error. + pub error: io::Error, +} + +#[cfg(unix)] +impl std::fmt::Display for CreateDirError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {}", self.path.maybe_quote(), self.error) + } +} + +#[cfg(unix)] +impl std::error::Error for CreateDirError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.error) + } +} + +#[cfg(unix)] +impl From for io::Error { + fn from(e: CreateDirError) -> Self { + e.error + } +} + impl AsRawFd for DirFd { fn as_raw_fd(&self) -> RawFd { self.fd.as_raw_fd() @@ -1453,6 +1497,33 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_create_dir_all_safe_reports_failing_component() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("file"); + fs::write(&file_path, "content").unwrap(); + + // The failing component is the file, not the whole requested path. + let err = create_dir_all_safe(&file_path.join("a/b"), 0o755).unwrap_err(); + assert_eq!(err.path, file_path); + assert_eq!(err.error.raw_os_error(), Some(libc::ENOTDIR)); + } + + #[test] + fn test_create_dir_all_safe_dangling_symlink_reports_eexist() { + let temp_dir = TempDir::new().unwrap(); + let link_path = temp_dir.path().join("dangling"); + symlink(temp_dir.path().join("nonexistent"), &link_path).unwrap(); + + // The name exists, so GNU reports EEXIST rather than the ENOENT of the + // unresolvable target. + let err = create_dir_all_safe(&link_path.join("sub"), 0o755).unwrap_err(); + assert_eq!(err.path, link_path); + assert_eq!(err.error.raw_os_error(), Some(libc::EEXIST)); + assert!(link_path.is_symlink()); + assert!(!temp_dir.path().join("nonexistent").exists()); + } + #[test] fn test_create_dir_all_safe_nested_symlink_in_path() { let temp_dir = TempDir::new().unwrap(); diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 6832c6edbdb..cb1b6ab9b6c 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2887,13 +2887,18 @@ fn test_install_d_dangling_symlink_in_path_errors() { at.write("file.txt", "hello"); - // install -D file.txt dangling/subdir/file.txt should fail + // install -D file.txt dangling/subdir/file.txt should fail, naming the + // dangling component, like GNU does. scene .ucmd() .args(&["-D", "-m", "644"]) .arg(at.plus("file.txt")) .arg(at.plus("dangling/subdir/file.txt")) - .fails(); + .fails() + .stderr_contains(format!( + "cannot create directory '{}': File exists", + at.plus_as_string("dangling") + )); // The dangling symlink must not have been replaced with a real directory assert!( @@ -2906,6 +2911,59 @@ fn test_install_d_dangling_symlink_in_path_errors() { ); } +#[test] +#[cfg(unix)] +fn test_install_directory_dangling_symlink_errors() { + // `install -d` must not follow a dangling symlink and create its target. + use std::os::unix::fs::symlink; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + symlink("nonexistent", at.plus("dangling")).unwrap(); + + for target in ["dangling", "dangling/sub", "dangling/a/b/c"] { + scene + .ucmd() + .args(&["-d", target]) + .fails() + .stderr_only("install: cannot create directory 'dangling': File exists\n"); + + assert!( + at.plus("dangling").is_symlink(), + "Dangling symlink must not be replaced with a real directory" + ); + assert!( + !at.plus("nonexistent").exists(), + "The symlink target must not have been created" + ); + } +} + +#[test] +#[cfg(unix)] +fn test_install_leading_dir_on_non_directory_errors() { + // A plain file in the middle of the path: the failing component is named, + // with the errno of descending into it. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.touch("regular"); + at.write("file.txt", "hello"); + + scene + .ucmd() + .args(&["-D", "file.txt", "regular/sub/file.txt"]) + .fails() + .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + + scene + .ucmd() + .args(&["-d", "regular/sub"]) + .fails() + .stderr_only("install: cannot create directory 'regular': Not a directory\n"); +} + #[test] #[cfg(target_os = "linux")] fn test_install_set_owner_nonexistent_uid_and_gid() { From 5e4fc68d5626acc622cab67da4cf1780eacdf8c3 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Mon, 21 Sep 2026 00:41:44 -0700 Subject: [PATCH 2/7] install: blame the ancestor only when it is part of the requested path --- src/uucore/src/lib/features/safe_traversal.rs | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 25eb0927e9b..a1ab896a93d 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -644,23 +644,34 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu #[cfg(unix)] pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result { let (existing_ancestor, components_to_create) = find_existing_ancestor(path); - let mut dir_fd = DirFd::open(&existing_ancestor, SymlinkBehavior::Follow).map_err(|error| { - CreateDirError { - path: existing_ancestor.clone(), - error, + + // `components_to_create` is the tail of `path`, so dropping the components + // that were not reached yields the prefix that failed. + let failing_prefix = |index: usize| { + let mut failed = path.to_path_buf(); + for _ in index + 1..components_to_create.len() { + failed.pop(); } + failed + }; + + // Failing to descend into the existing ancestor is blamed on the ancestor + // when it is part of the requested path, and on the first component we + // would have created when it is the implicit "." or "/" - the caller never + // named the current directory, so GNU does not name it either. + let mut dir_fd = DirFd::open(&existing_ancestor, SymlinkBehavior::Follow).map_err(|error| { + let path = if path.starts_with(&existing_ancestor) { + existing_ancestor.clone() + } else { + failing_prefix(0) + }; + CreateDirError { path, error } })?; for (index, component) in components_to_create.iter().enumerate() { dir_fd = open_or_create_subdir(&dir_fd, component.as_os_str(), mode).map_err(|error| { - // `components_to_create` is the tail of `path`, so dropping the - // components that were not reached yields the failing prefix. - let mut failed = path.to_path_buf(); - for _ in index + 1..components_to_create.len() { - failed.pop(); - } CreateDirError { - path: failed, + path: failing_prefix(index), error, } })?; From f088cbd4d34355728c63899cd447c1a8627b0b99 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Mon, 21 Sep 2026 01:22:49 -0700 Subject: [PATCH 3/7] install: keep -d path-based and blame the unsearchable parent --- src/uu/install/src/install.rs | 25 +++------ src/uucore/src/lib/features/safe_traversal.rs | 36 ++++++++---- tests/by-util/test_install.rs | 55 ++++++++----------- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 1b389f4a28a..74f58ba6bbe 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -506,22 +506,15 @@ fn directory(paths: &[OsString], b: &Behavior) -> UResult<()> { // // NOTE: the GNU "install" sets the expected mode only for the // target directory. All created ancestor directories will have - // the default mode. Hence it is safe to create the whole chain - // with the default mode and then only modify the target's dir mode. - #[cfg(unix)] - { - // 0o777 is what `fs::create_dir_all` requests; the kernel - // applies the umask on top of it. - if let Err(e) = create_dir_all_safe(&path_to_create, 0o777) { - show!(InstallError::CreateDirFailed(e.path, e.error)); - continue; - } - } - #[cfg(not(unix))] - if let Err(e) = fs::create_dir_all(&path_to_create) - .map_err(|e| InstallError::CreateDirFailed(path_to_create.to_path_buf(), e)) - { - show!(e); + // the default mode. Hence it is safe to use fs::create_dir_all + // and then only modify the target's dir mode. + // + // This stays path-based on purpose. Anchoring the walk to + // directory fds needs read permission on each existing ancestor, + // while mkdir only needs write and execute, so an fd walk fails on + // write-only directories where GNU succeeds. + if let Err(e) = fs::create_dir_all(&path_to_create) { + show!(InstallError::CreateDirFailed(path_to_create.clone(), e)); continue; } diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index a1ab896a93d..70e2ccabb4a 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -613,6 +613,26 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu } } +/// Pick the path a GNU utility would name for a failed directory creation. +/// +/// GNU reports the component whose creation failed, unless the parent cannot be +/// searched at all - then the parent is the real obstacle and gets named. That +/// is the difference between `install -d r--/x`, which reports `r--`, and +/// `install -d r-x/x`, which reports `r-x/x`. +#[cfg(unix)] +fn blame(failed: PathBuf) -> PathBuf { + let Some(parent) = failed.parent().filter(|p| !p.as_os_str().is_empty()) else { + return failed; + }; + let searchable = CString::new(parent.as_os_str().as_bytes()) + .is_ok_and(|c| unsafe { libc::access(c.as_ptr(), libc::X_OK) } == 0); + if searchable { + failed + } else { + parent.to_path_buf() + } +} + /// Safely create all parent directories for a path using directory file descriptors. /// This prevents symlink race conditions by anchoring all operations to directory fds. /// @@ -652,20 +672,14 @@ pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result Date: Wed, 30 Sep 2026 19:02:33 -0700 Subject: [PATCH 4/7] uucore: drop the unused CreateDirError to io::Error conversion Nothing converts a CreateDirError into an io::Error, and doing so would silently drop the path the type exists to carry. Also document why the access(2) call in blame() is sound. --- src/uucore/src/lib/features/safe_traversal.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 70e2ccabb4a..d39e4a6f375 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -624,8 +624,10 @@ fn blame(failed: PathBuf) -> PathBuf { let Some(parent) = failed.parent().filter(|p| !p.as_os_str().is_empty()) else { return failed; }; - let searchable = CString::new(parent.as_os_str().as_bytes()) - .is_ok_and(|c| unsafe { libc::access(c.as_ptr(), libc::X_OK) } == 0); + let searchable = CString::new(parent.as_os_str().as_bytes()).is_ok_and(|c| { + // SAFETY: `c` is a valid NUL-terminated string that outlives the call. + unsafe { libc::access(c.as_ptr(), libc::X_OK) == 0 } + }); if searchable { failed } else { @@ -722,13 +724,6 @@ impl std::error::Error for CreateDirError { } } -#[cfg(unix)] -impl From for io::Error { - fn from(e: CreateDirError) -> Self { - e.error - } -} - impl AsRawFd for DirFd { fn as_raw_fd(&self) -> RawFd { self.fd.as_raw_fd() From 28616c7ddab5a717bee1f28a51690c82556027b1 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Wed, 30 Sep 2026 21:04:07 -0700 Subject: [PATCH 5/7] install: name the failing component for -d as well `install -d` creates its directories by path, so it still reported the whole path when that failed, e.g. 'dangling/sub' where GNU names 'dangling'. Name the first component that is not an existing directory, through the same blame logic create_dir_all_safe uses for -D. --- src/uu/install/src/install.rs | 12 +++++-- src/uucore/src/lib/features/safe_traversal.rs | 33 ++++++++++++++----- tests/by-util/test_install.rs | 16 +++++++++ 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 74f58ba6bbe..425702f5c19 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.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 (ToDO) rwxr sourcepath targetpath Isnt uioerror matchpathcon +// spell-checker:ignore (ToDO) rwxr sourcepath targetpath Isnt uioerror matchpathcon ENOTDIR mod mode; @@ -28,7 +28,9 @@ use uucore::error::{FromIo, UError, UResult, UUsageError, strip_errno}; use uucore::fs::{are_files_identical, dir_strip_dot_for_creation}; use uucore::perms::{Verbosity, VerbosityLevel, wrap_chown}; #[cfg(unix)] -use uucore::safe_traversal::{DirFd, SymlinkBehavior, create_dir_all_safe}; +use uucore::safe_traversal::{ + DirFd, SymlinkBehavior, create_dir_all_safe, failed_create_dir_prefix, +}; #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] use uucore::selinux::{ SeLinuxError, contexts_differ, get_selinux_security_context, is_selinux_enabled, @@ -514,7 +516,11 @@ fn directory(paths: &[OsString], b: &Behavior) -> UResult<()> { // while mkdir only needs write and execute, so an fd walk fails on // write-only directories where GNU succeeds. if let Err(e) = fs::create_dir_all(&path_to_create) { - show!(InstallError::CreateDirFailed(path_to_create.clone(), e)); + #[cfg(unix)] + let failed = failed_create_dir_prefix(&path_to_create); + #[cfg(not(unix))] + let failed = path_to_create.clone(); + show!(InstallError::CreateDirFailed(failed, e)); continue; } diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index d39e4a6f375..bb3cd2ec479 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -667,15 +667,7 @@ fn blame(failed: PathBuf) -> PathBuf { pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result { let (existing_ancestor, components_to_create) = find_existing_ancestor(path); - // `components_to_create` is the tail of `path`, so dropping the components - // that were not reached yields the prefix that failed. - let failing_prefix = |index: usize| { - let mut failed = path.to_path_buf(); - for _ in index + 1..components_to_create.len() { - failed.pop(); - } - blame(failed) - }; + let failing_prefix = |index: usize| failing_prefix(path, &components_to_create, index); let mut dir_fd = DirFd::open(&existing_ancestor, SymlinkBehavior::Follow).map_err(|error| { CreateDirError { @@ -696,6 +688,29 @@ pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result PathBuf { + let (_, components_to_create) = find_existing_ancestor(path); + failing_prefix(path, &components_to_create, 0) +} + +/// The prefix of `path` ending at `components_to_create[index]`, as GNU names it. +/// +/// `components_to_create` is the tail of `path`, so dropping the components +/// after `index` yields the prefix that failed. +#[cfg(unix)] +fn failing_prefix(path: &Path, components_to_create: &[OsString], index: usize) -> PathBuf { + let mut failed = path.to_path_buf(); + for _ in index + 1..components_to_create.len() { + failed.pop(); + } + blame(failed) +} + /// Failure of [`create_dir_all_safe`], naming the path component that failed. /// /// GNU utilities report the first path prefix they could not create, not the diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index d8e1c2c274a..8670f992083 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2900,6 +2900,13 @@ fn test_install_d_dangling_symlink_in_path_errors() { at.plus_as_string("dangling") )); + // `install -d` creates by path, but names the same component. + scene + .ucmd() + .args(&["-d", "dangling/subdir"]) + .fails() + .stderr_only("install: cannot create directory 'dangling': File exists\n"); + // The dangling symlink must not have been replaced with a real directory assert!( at.plus("dangling").is_symlink(), @@ -2933,6 +2940,15 @@ fn test_install_leading_dir_blames_failing_component() { .args(&["-D", "file.txt", "regular/sub/deeper/file.txt"]) .fails() .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + + // `install -d` creates by path, but names the same component. + for dir in ["regular/sub", "regular/sub/deeper"] { + scene + .ucmd() + .args(&["-d", dir]) + .fails() + .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + } } #[test] From b412f8faae71e8c8425162db4d56828a65354d05 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Wed, 30 Sep 2026 21:39:25 -0700 Subject: [PATCH 6/7] install: test naming an unsearchable parent and a symlink loop The parent-versus-component choice in blame() had no test: cover a parent that cannot be searched (named) against one that can (the component is named), and a symlink loop in the leading directories, for -D and -d. --- tests/by-util/test_install.rs | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 8670f992083..bcf2d1d9ccd 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2941,6 +2941,19 @@ fn test_install_leading_dir_blames_failing_component() { .fails() .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + // A symlink loop is named the same way. + at.symlink_file("loop", "loop"); + scene + .ucmd() + .args(&["-D", "file.txt", "loop/sub/file.txt"]) + .fails() + .stderr_only( + "install: cannot create directory 'loop': Too many levels of symbolic links\n", + ); + scene.ucmd().args(&["-d", "loop/sub"]).fails().stderr_only( + "install: cannot create directory 'loop': Too many levels of symbolic links\n", + ); + // `install -d` creates by path, but names the same component. for dir in ["regular/sub", "regular/sub/deeper"] { scene @@ -2973,6 +2986,39 @@ fn test_install_directory_in_write_only_directory() { scene.ucmd().args(&["-d", "wx/sub"]).succeeds(); } +#[test] +#[cfg(unix)] +fn test_install_leading_dir_blames_unsearchable_parent() { + // A parent that cannot be searched is what gets named, like GNU does; one + // that can be searched but not written leaves the component to blame. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + if geteuid().is_root() { + println!("Test skipped; root ignores directory permissions"); + return; + } + + at.touch("f"); + at.mkdir("r--"); + fs::set_permissions(at.plus("r--"), fs::Permissions::from_mode(0o444)).unwrap(); + at.mkdir("r-x"); + fs::set_permissions(at.plus("r-x"), fs::Permissions::from_mode(0o555)).unwrap(); + + for args in [&["-d", "r--/x"][..], &["-D", "f", "r--/x/f"]] { + scene + .ucmd() + .args(args) + .fails() + .stderr_only("install: cannot create directory 'r--': Permission denied\n"); + } + scene + .ucmd() + .args(&["-D", "f", "r-x/x/f"]) + .fails() + .stderr_only("install: cannot create directory 'r-x/x': Permission denied\n"); +} + #[test] #[cfg(target_os = "linux")] fn test_install_set_owner_nonexistent_uid_and_gid() { From 5867bbefe128b0b81be172439c35ba021d181e27 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Wed, 30 Sep 2026 23:51:57 -0700 Subject: [PATCH 7/7] tests/install: expect musl's and bionic's ELOOP text in the leading-dir test --- tests/by-util/test_install.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index bcf2d1d9ccd..f2cbe21d73d 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2941,18 +2941,24 @@ fn test_install_leading_dir_blames_failing_component() { .fails() .stderr_only("install: cannot create directory 'regular': Not a directory\n"); - // A symlink loop is named the same way. + // A symlink loop is named the same way, with the libc's ELOOP text. + #[cfg(all(not(target_env = "musl"), not(target_os = "android")))] + let expected = "install: cannot create directory 'loop': Too many levels of symbolic links\n"; + #[cfg(all(not(target_env = "musl"), target_os = "android"))] + let expected = "install: cannot create directory 'loop': Too many symbolic links encountered\n"; + #[cfg(all(target_env = "musl", not(target_os = "android")))] + let expected = "install: cannot create directory 'loop': Symbolic link loop\n"; at.symlink_file("loop", "loop"); scene .ucmd() .args(&["-D", "file.txt", "loop/sub/file.txt"]) .fails() - .stderr_only( - "install: cannot create directory 'loop': Too many levels of symbolic links\n", - ); - scene.ucmd().args(&["-d", "loop/sub"]).fails().stderr_only( - "install: cannot create directory 'loop': Too many levels of symbolic links\n", - ); + .stderr_only(expected); + scene + .ucmd() + .args(&["-d", "loop/sub"]) + .fails() + .stderr_only(expected); // `install -d` creates by path, but names the same component. for dir in ["regular/sub", "regular/sub/deeper"] {