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..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, @@ -72,7 +74,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()))] @@ -508,10 +510,17 @@ fn directory(paths: &[OsString], b: &Behavior) -> UResult<()> { // 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()), - ) { - show!(e); + // + // 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) { + #[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; } @@ -658,11 +667,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 +683,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 +747,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..bb3cd2ec479 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) { @@ -622,6 +613,28 @@ 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| { + // 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 { + 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. /// @@ -647,19 +660,85 @@ 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)?; +pub fn create_dir_all_safe(path: &Path, mode: u32) -> Result { + let (existing_ancestor, components_to_create) = find_existing_ancestor(path); + + 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 { + path: failing_prefix(0), + error, + } + })?; - for component in &components_to_create { - dir_fd = open_or_create_subdir(&dir_fd, component.as_os_str(), mode)?; + 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| { + CreateDirError { + path: failing_prefix(index), + error, + } + })?; } Ok(dir_fd) } +/// The prefix of `path` to report when creating it by path has failed. +/// +/// For callers that cannot use [`create_dir_all_safe`]: names the first +/// component that is not an existing directory, as that function does. +#[cfg(unix)] +pub fn failed_create_dir_prefix(path: &Path) -> 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 +/// 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) + } +} + impl AsRawFd for DirFd { fn as_raw_fd(&self) -> RawFd { self.fd.as_raw_fd() @@ -1453,6 +1532,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..f2cbe21d73d 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2887,13 +2887,25 @@ 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") + )); + + // `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!( @@ -2906,6 +2918,113 @@ fn test_install_d_dangling_symlink_in_path_errors() { ); } +#[test] +#[cfg(unix)] +fn test_install_leading_dir_blames_failing_component() { + // 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", "file.txt", "regular/sub/deeper/file.txt"]) + .fails() + .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + + // 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(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"] { + scene + .ucmd() + .args(&["-d", dir]) + .fails() + .stderr_only("install: cannot create directory 'regular': Not a directory\n"); + } +} + +#[test] +#[cfg(unix)] +fn test_install_directory_in_write_only_directory() { + // mkdir only needs write and execute on the parent, so `install -d` must + // keep working in a directory it cannot read. `-D` still fails here, see + // the fd-based walk in create_dir_all_safe. + use std::os::unix::fs::PermissionsExt; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + if geteuid().is_root() { + println!("Test skipped; root ignores directory permissions"); + return; + } + + at.mkdir("wx"); + fs::set_permissions(at.plus("wx"), fs::Permissions::from_mode(0o300)).unwrap(); + + 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() {