From 3306a0615ba027f9c84d50aa184823b95aa8c556 Mon Sep 17 00:00:00 2001 From: rm-dr <96270320+rm-dr@users.noreply.github.com> Date: Sun, 24 Nov 2024 09:07:49 -0800 Subject: [PATCH 1/3] cp: add subdir permissions test --- tests/by-util/test_cp.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 19fbc5ca717..fc56dc3160e 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -3365,6 +3365,29 @@ fn test_copy_dir_preserve_permissions() { assert_metadata_eq!(metadata1, metadata2); } +/// cp should preserve attributes of subdirectories when copying recursively. +#[cfg(all(not(windows), not(target_os = "freebsd"), not(target_os = "openbsd")))] +#[test] +fn test_copy_dir_preserve_subdir_permissions() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("a1"); + at.mkdir("a1/a2"); + // Use different permissions for a better test + at.set_mode("a1/a2", 0o0555); + at.set_mode("a1", 0o0777); + + ucmd.args(&["-p", "-r", "a1", "b1"]) + .succeeds() + .no_stderr() + .no_stdout(); + + // Make sure everything is preserved + assert!(at.dir_exists("b1")); + assert!(at.dir_exists("b1/a2")); + assert_metadata_eq!(at.metadata("a1"), at.metadata("b1")); + assert_metadata_eq!(at.metadata("a1/a2"), at.metadata("b1/a2")); +} + /// Test for preserving permissions when copying a directory, even in /// the face of an inaccessible file in that directory. #[cfg(all(not(windows), not(target_os = "freebsd"), not(target_os = "openbsd")))] @@ -5616,7 +5639,7 @@ mod link_deref { // which could be problematic if we aim to preserve ownership or mode. For example, when // copying a directory, the destination directory could temporarily be setgid on some filesystems. // This temporary setgid status could grant access to other users who share the same group -// ownership as the newly created directory.To mitigate this issue, when creating a directory we +// ownership as the newly created directory. To mitigate this issue, when creating a directory we // disable these excessive permissions. #[test] #[cfg(unix)] From 9d6ae1c1ac4ecdcbed4778cdd070dc61a766f76e Mon Sep 17 00:00:00 2001 From: rm-dr <96270320+rm-dr@users.noreply.github.com> Date: Mon, 2 Dec 2024 18:43:29 -0800 Subject: [PATCH 2/3] cp: minor cleanup --- src/uu/cp/src/copydir.rs | 23 ++++++++++++++--------- src/uu/cp/src/cp.rs | 2 +- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 3bfc5524913..fff8b5dcd48 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -23,7 +23,7 @@ use uucore::fs::{ use uucore::show; use uucore::show_error; use uucore::uio_error; -use walkdir::{DirEntry, WalkDir}; +use walkdir::WalkDir; use crate::{ aligned_ancestors, context_for, copy_attributes, copy_file, copy_link, CopyResult, Error, @@ -162,17 +162,18 @@ struct Entry { } impl Entry { - fn new( + fn new>( context: &Context, - direntry: &DirEntry, + source: A, no_target_dir: bool, ) -> Result { - let source_relative = direntry.path().to_path_buf(); + let source = source.as_ref(); + let source_relative = source.to_path_buf(); let source_absolute = context.current_dir.join(&source_relative); let mut descendant = get_local_to_root_parent(&source_absolute, context.root_parent.as_deref())?; if no_target_dir { - let source_is_dir = direntry.path().is_dir(); + let source_is_dir = source.is_dir(); if path_ends_with_terminator(context.target) && source_is_dir { if let Err(e) = std::fs::create_dir_all(context.target) { eprintln!("Failed to create directory: {e}"); @@ -213,6 +214,7 @@ where // `path.ends_with(".")` does not seem to work path.as_ref().display().to_string().ends_with("/.") } + #[allow(clippy::too_many_arguments)] /// Copy a single entry during a directory traversal. fn copy_direntry( @@ -246,7 +248,7 @@ fn copy_direntry( if target_is_file { return Err("cannot overwrite non-directory with directory".into()); } else { - build_dir(options, &local_to_target, false)?; + build_dir(&local_to_target, false, options)?; if options.verbose { println!("{}", context_for(&source_relative, &local_to_target)); } @@ -371,7 +373,7 @@ pub(crate) fn copy_directory( let tmp = if options.parents { if let Some(parent) = root.parent() { let new_target = target.join(parent); - build_dir(options, &new_target, true)?; + build_dir(&new_target, true, options)?; if options.verbose { // For example, if copying file `a/b/c` and its parents // to directory `d/`, then print @@ -410,7 +412,8 @@ pub(crate) fn copy_directory( { match direntry_result { Ok(direntry) => { - let entry = Entry::new(&context, &direntry, options.no_target_dir)?; + let entry = Entry::new(&context, direntry.path(), options.no_target_dir)?; + copy_direntry( progress_bar, entry, @@ -475,9 +478,10 @@ pub fn path_has_prefix(p1: &Path, p2: &Path) -> io::Result { /// if they do not already exist. // we need to allow unused_variable since `options` might be unused in non unix systems #[allow(unused_variables)] -fn build_dir(options: &Options, path: &PathBuf, recursive: bool) -> CopyResult<()> { +fn build_dir(path: &PathBuf, recursive: bool, options: &Options) -> CopyResult<()> { let mut builder = fs::DirBuilder::new(); builder.recursive(recursive); + // To prevent unauthorized access before the folder is ready, // exclude certain permissions if ownership or special mode bits // could potentially change. @@ -498,6 +502,7 @@ fn build_dir(options: &Options, path: &PathBuf, recursive: bool) -> CopyResult<( let mode = !excluded_perms & 0o777; //use only the last three octet bits std::os::unix::fs::DirBuilderExt::mode(&mut builder, mode); } + builder.create(path)?; Ok(()) } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 41686f5bf4f..c1ebc20bf80 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -174,7 +174,7 @@ pub enum CopyMode { /// For full compatibility with GNU, these options should also combine. We /// currently only do a best effort imitation of that behavior, because it is /// difficult to achieve in clap, especially with `--no-preserve`. -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone, Copy)] pub struct Attributes { #[cfg(unix)] pub ownership: Preserve, From aac753e17f9f99cfc3a68fc5fcbed14197f292be Mon Sep 17 00:00:00 2001 From: rm-dr <96270320+rm-dr@users.noreply.github.com> Date: Mon, 2 Dec 2024 18:46:15 -0800 Subject: [PATCH 3/3] cp: fix subdir permissions --- src/uu/cp/src/copydir.rs | 123 +++++++++++++++++++++++++++++++++++---- 1 file changed, 112 insertions(+), 11 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index fff8b5dcd48..bd81a39f5da 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -23,7 +23,7 @@ use uucore::fs::{ use uucore::show; use uucore::show_error; use uucore::uio_error; -use walkdir::WalkDir; +use walkdir::{DirEntry, WalkDir}; use crate::{ aligned_ancestors, context_for, copy_attributes, copy_file, copy_link, CopyResult, Error, @@ -79,6 +79,12 @@ fn get_local_to_root_parent( } } +/// Given an iterator, return all its items except the last. +fn skip_last(mut iter: impl Iterator) -> impl Iterator { + let last = iter.next(); + iter.scan(last, |state, item| std::mem::replace(state, Some(item))) +} + /// Paths that are invariant throughout the traversal when copying a directory. struct Context<'a> { /// The current working directory at the time of starting the traversal. @@ -248,7 +254,7 @@ fn copy_direntry( if target_is_file { return Err("cannot overwrite non-directory with directory".into()); } else { - build_dir(&local_to_target, false, options)?; + build_dir(&local_to_target, false, options, Some(&source_absolute))?; if options.verbose { println!("{}", context_for(&source_relative, &local_to_target)); } @@ -373,7 +379,7 @@ pub(crate) fn copy_directory( let tmp = if options.parents { if let Some(parent) = root.parent() { let new_target = target.join(parent); - build_dir(&new_target, true, options)?; + build_dir(&new_target, true, options, None)?; if options.verbose { // For example, if copying file `a/b/c` and its parents // to directory `d/`, then print @@ -405,6 +411,9 @@ pub(crate) fn copy_directory( Err(e) => return Err(format!("failed to get current directory {e}").into()), }; + // The directory we were in during the previous iteration + let mut last_iter: Option = None; + // Traverse the contents of the directory, copying each one. for direntry_result in WalkDir::new(root) .same_file_system(options.one_file_system) @@ -423,23 +432,93 @@ pub(crate) fn copy_directory( copied_destinations, copied_files, )?; + + // We omit certain permissions when creating directories + // to prevent other users from accessing them before they're done. + // We thus need to fix the permissions of each directory we copy + // once it's contents are ready. + // This "fixup" is implemented here in a memory-efficient manner. + // + // We detect iterations where we "walk up" the directory tree, + // and fix permissions on all the directories we exited. + // (Note that there can be more than one! We might step out of + // `./a/b/c` into `./a/`, in which case we'll need to fix the + // permissions of both `./a/b/c` and `./a/b`, in that order.) + if direntry.file_type().is_dir() { + // If true, last_iter is not a parent of this iter. + // The means we just exited a directory. + let went_up = if let Some(last_iter) = &last_iter { + last_iter.path().strip_prefix(direntry.path()).is_ok() + } else { + false + }; + + if went_up { + // Compute the "difference" between `last_iter` and `direntry`. + // For example, if... + // - last_iter = `a/b/c/d` + // - direntry = `a/b` + // then diff = `c/d` + // + // All the unwraps() here are unreachable. + let last_iter = last_iter.as_ref().unwrap(); + let diff = last_iter.path().strip_prefix(direntry.path()).unwrap(); + + // Fix permissions for every entry in `diff`, inside-out. + // We skip the last directory (which will be `.`) because + // its permissions will be fixed when we walk _out_ of it. + // (at this point, we might not be done copying `.`!) + for p in skip_last(diff.ancestors()) { + let src = direntry.path().join(p); + let entry = Entry::new(&context, &src, options.no_target_dir)?; + + copy_attributes( + &entry.source_absolute, + &entry.local_to_target, + &options.attributes, + )?; + } + } + + last_iter = Some(direntry); + } } + // Print an error message, but continue traversing the directory. Err(e) => show_error!("{}", e), } } - // Copy the attributes from the root directory to the target directory. + // Handle final directory permission fixes. + // This is almost the same as the permission-fixing code above, + // with minor differences (commented) + if let Some(last_iter) = last_iter { + let diff = last_iter.path().strip_prefix(root).unwrap(); + + // Do _not_ skip `.` this time, since we know we're done. + // This is where we fix the permissions of the top-level + // directory we just copied. + for p in diff.ancestors() { + let src = root.join(p); + let entry = Entry::new(&context, &src, options.no_target_dir)?; + + copy_attributes( + &entry.source_absolute, + &entry.local_to_target, + &options.attributes, + )?; + } + } + + // Also fix permissions for parent directories, + // if we were asked to create them. if options.parents { let dest = target.join(root.file_name().unwrap()); - copy_attributes(root, dest.as_path(), &options.attributes)?; for (x, y) in aligned_ancestors(root, dest.as_path()) { if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) { copy_attributes(&src, y, &options.attributes)?; } } - } else { - copy_attributes(root, target, &options.attributes)?; } Ok(()) @@ -472,13 +551,21 @@ pub fn path_has_prefix(p1: &Path, p2: &Path) -> io::Result { /// Builds a directory at the specified path with the given options. /// /// # Notes -/// - It excludes certain permissions if ownership or special mode bits could -/// potentially change. +/// - If `copy_attributes_from` is `Some`, the new directory's attributes will be +/// copied from the provided file. Otherwise, the new directory will have the default +/// attributes for the current user. +/// - This method excludes certain permissions if ownership or special mode bits could +/// potentially change. (See `test_dir_perm_race_with_preserve_mode_and_ownership``) /// - The `recursive` flag determines whether parent directories should be created /// if they do not already exist. // we need to allow unused_variable since `options` might be unused in non unix systems #[allow(unused_variables)] -fn build_dir(path: &PathBuf, recursive: bool, options: &Options) -> CopyResult<()> { +fn build_dir( + path: &PathBuf, + recursive: bool, + options: &Options, + copy_attributes_from: Option<&Path>, +) -> CopyResult<()> { let mut builder = fs::DirBuilder::new(); builder.recursive(recursive); @@ -487,6 +574,9 @@ fn build_dir(path: &PathBuf, recursive: bool, options: &Options) -> CopyResult<( // could potentially change. #[cfg(unix)] { + use crate::Preserve; + use std::os::unix::fs::PermissionsExt; + // we need to allow trivial casts here because some systems like linux have u32 constants in // in libc while others don't. #[allow(clippy::unnecessary_cast)] @@ -498,7 +588,18 @@ fn build_dir(path: &PathBuf, recursive: bool, options: &Options) -> CopyResult<( } else { 0 } as u32; - excluded_perms |= uucore::mode::get_umask(); + + let umask = if copy_attributes_from.is_some() + && matches!(options.attributes.mode, Preserve::Yes { .. }) + { + !fs::symlink_metadata(copy_attributes_from.unwrap())? + .permissions() + .mode() + } else { + uucore::mode::get_umask() + }; + + excluded_perms |= umask; let mode = !excluded_perms & 0o777; //use only the last three octet bits std::os::unix::fs::DirBuilderExt::mode(&mut builder, mode); }