From 8e1965fa39b17f087e9efef252973cb4d5d770cc Mon Sep 17 00:00:00 2001 From: rm-dr <96270320+rm-dr@users.noreply.github.com> Date: Fri, 29 Nov 2024 22:07:58 -0800 Subject: [PATCH] cp: efficient permission fixup --- src/uu/cp/src/copydir.rs | 133 ++++++++++++++++++++++++++++++++------- 1 file changed, 109 insertions(+), 24 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index e6ba4064021..415da094604 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -79,6 +79,33 @@ fn get_local_to_root_parent( } } +/// Return the longest parent shared by two paths. +/// If `a` and `b` to not share a parent, return `None`. +fn common, B: AsRef>(path_a: A, path_b: B) -> Option { + let path_a = path_a.as_ref().components(); + let path_b = path_b.as_ref().components(); + + let mut nonempty = false; + let mut out = PathBuf::new(); + + for (a, b) in path_a.zip(path_b) { + if a != b { + break; + } + + out.push(a); + nonempty = true; + } + + return nonempty.then_some(out); +} + +/// 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. @@ -162,17 +189,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 +241,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( @@ -223,7 +252,6 @@ fn copy_direntry( preserve_hard_links: bool, copied_destinations: &HashSet, copied_files: &mut HashMap, - dirs_with_attrs_to_fix: &mut Vec<(PathBuf, PathBuf)>, ) -> CopyResult<()> { let Entry { source_absolute, @@ -251,10 +279,6 @@ fn copy_direntry( if options.verbose { println!("{}", context_for(&source_relative, &local_to_target)); } - - // `build_dir` doesn't set fully set attributes, - // we'll need to fix them later. - dirs_with_attrs_to_fix.push((source_absolute, local_to_target)); return Ok(()); } } @@ -408,15 +432,8 @@ pub(crate) fn copy_directory( Err(e) => return Err(format!("failed to get current directory {e}").into()), }; - // We omit certain permissions when creating dirs - // to prevent other uses from accessing them before they're done - // (race condition). - // - // As such, we need to go back through the dirs we copied and - // fix these permissions. - // - // This is a vec of (old_path, new_path) - let mut dirs_with_attrs_to_fix: Vec<(PathBuf, PathBuf)> = Vec::new(); + // 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) @@ -425,7 +442,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, @@ -434,20 +452,87 @@ pub(crate) fn copy_directory( preserve_hard_links, copied_destinations, copied_files, - &mut dirs_with_attrs_to_fix, )?; + + // We omit certain permissions when creating directories + // to prevent other uses 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. + // + // Here, 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 { + direntry.path().strip_prefix(&last_iter.path()).is_err() + } 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` + let last_iter = last_iter.as_ref().unwrap(); + let common = common(direntry.path(), last_iter.path()).unwrap(); + let diff = last_iter.path().strip_prefix(&common).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 = common.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), } } - // Fix permissions for all directories we created - for (src, tgt) in dirs_with_attrs_to_fix { - copy_attributes(&src, &tgt, &options.attributes)?; + // Handle final directory permission fixes. + // This is almost the same as the permisson-fixing code above, + // with minor differences (commented) + if let Some(last_iter) = last_iter { + let common = common(root, last_iter.path()).unwrap(); + let diff = last_iter.path().strip_prefix(&common).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 = common.join(p); + let entry = Entry::new(&context, &src, options.no_target_dir)?; + + copy_attributes( + &entry.source_absolute, + &entry.local_to_target, + &options.attributes, + )?; + } } - // Copy the attributes from the root directory to the target directory. + // Also fix permissions for parent directories, + // if we were asked to create them. if options.parents { let dest = target.join(root.file_name().unwrap()); for (x, y) in aligned_ancestors(root, dest.as_path()) {