From 12a1c87cb8e72f0df8b03594f425f8c98268ec1c Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 17 Jun 2021 22:26:13 +0200 Subject: [PATCH 1/4] cp: improve symlink handling --- src/uu/cp/src/cp.rs | 80 ++++++++++++++++++++++------------------ tests/by-util/test_cp.rs | 26 +++++++++++++ 2 files changed, 70 insertions(+), 36 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 2ebbeddb0..7e7bcca4c 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -48,7 +48,6 @@ use std::path::{Path, PathBuf, StripPrefixError}; use std::str::FromStr; use std::string::ToString; use uucore::backup_control::{self, BackupMode}; -use uucore::fs::resolve_relative_path; use uucore::fs::{canonicalize, CanonicalizeMode}; use walkdir::WalkDir; @@ -198,7 +197,6 @@ pub struct Options { copy_contents: bool, copy_mode: CopyMode, dereference: bool, - no_dereference: bool, no_target_dir: bool, one_file_system: bool, overwrite: OverwriteMode, @@ -641,11 +639,12 @@ impl Options { attributes_only: matches.is_present(OPT_ATTRIBUTES_ONLY), copy_contents: matches.is_present(OPT_COPY_CONTENTS), copy_mode: CopyMode::from_matches(matches), - dereference: matches.is_present(OPT_DEREFERENCE), // No dereference is set with -p, -d and --archive - no_dereference: matches.is_present(OPT_NO_DEREFERENCE) + dereference: !(matches.is_present(OPT_NO_DEREFERENCE) || matches.is_present(OPT_NO_DEREFERENCE_PRESERVE_LINKS) - || matches.is_present(OPT_ARCHIVE), + || matches.is_present(OPT_ARCHIVE) + || recursive) + || matches.is_present(OPT_DEREFERENCE), one_file_system: matches.is_present(OPT_ONE_FILE_SYSTEM), parents: matches.is_present(OPT_PARENTS), update: matches.is_present(OPT_UPDATE), @@ -896,7 +895,14 @@ fn copy_source( options: &Options, ) -> CopyResult<()> { let source_path = Path::new(&source); - if source_path.is_dir() { + // if no-dereference is enabled and this is a symlink, don't treat it as a directory + if source_path.is_dir() + && !(!options.dereference + && fs::symlink_metadata(source_path) + .unwrap() + .file_type() + .is_symlink()) + { // Copy as directory copy_directory(source, target, options) } else { @@ -937,7 +943,7 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR return Err(format!("omitting directory '{}'", root.display()).into()); } - let root_path = Path::new(&root).canonicalize()?; + let root_path = env::current_dir().unwrap().join(root); let root_parent = if target.exists() { root_path.parent() @@ -958,17 +964,15 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR #[cfg(any(windows, target_os = "redox"))] let mut hard_links: Vec<(String, u64)> = vec![]; - for path in WalkDir::new(root).same_file_system(options.one_file_system) { + for path in WalkDir::new(root) + .same_file_system(options.one_file_system) + .follow_links(options.dereference) + { let p = or_continue!(path); let is_symlink = fs::symlink_metadata(p.path())?.file_type().is_symlink(); - let path = if (options.no_dereference || options.dereference) && is_symlink { - // we are dealing with a symlink. Don't follow it - match env::current_dir() { - Ok(cwd) => cwd.join(resolve_relative_path(p.path())), - Err(e) => crash!(1, "failed to get current directory {}", e), - } - } else { - or_continue!(p.path().canonicalize()) + let path = match env::current_dir() { + Ok(cwd) => cwd.join(&p.path()), + Err(e) => crash!(1, "failed to get current directory {}", e), }; let local_to_root_parent = match root_parent { @@ -992,9 +996,10 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR }; let local_to_target = target.join(&local_to_root_parent); - - if path.is_dir() && !local_to_target.exists() { - or_continue!(fs::create_dir_all(local_to_target.clone())); + if is_symlink && !options.dereference { + copy_link(&path, &local_to_target)?; + } else if path.is_dir() && !local_to_target.exists() { + or_continue!(fs::create_dir_all(local_to_target)); } else if !path.is_dir() { if preserve_hard_links { let mut found_hard_link = false; @@ -1220,25 +1225,10 @@ fn copy_helper(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> #[cfg(target_os = "macos")] copy_on_write_macos(source, dest, options.reflink_mode)?; - #[cfg(target_os = "linux")] copy_on_write_linux(source, dest, options.reflink_mode)?; - } else if options.no_dereference && fs::symlink_metadata(&source)?.file_type().is_symlink() { - // Here, we will copy the symlink itself (actually, just recreate it) - let link = fs::read_link(&source)?; - let dest: Cow<'_, Path> = if dest.is_dir() { - match source.file_name() { - Some(name) => dest.join(name).into(), - None => crash!( - EXIT_ERR, - "cannot stat ‘{}’: No such file or directory", - source.display() - ), - } - } else { - dest.into() - }; - symlink_file(&link, &dest, &*context_for(&link, &dest))?; + } else if !options.dereference && fs::symlink_metadata(&source)?.file_type().is_symlink() { + copy_link(source, dest)?; } else if source.to_string_lossy() == "/dev/null" { /* workaround a limitation of fs::copy * https://github.com/rust-lang/rust/issues/79390 @@ -1255,6 +1245,24 @@ fn copy_helper(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> Ok(()) } +fn copy_link(source: &Path, dest: &Path) -> CopyResult<()> { + // Here, we will copy the symlink itself (actually, just recreate it) + let link = fs::read_link(&source)?; + let dest: Cow<'_, Path> = if dest.is_dir() { + match source.file_name() { + Some(name) => dest.join(name).into(), + None => crash!( + EXIT_ERR, + "cannot stat ‘{}’: No such file or directory", + source.display() + ), + } + } else { + dest.into() + }; + symlink_file(&link, &dest, &*context_for(&link, &dest)) +} + /// Copies `source` to `dest` using copy-on-write if possible. #[cfg(target_os = "linux")] fn copy_on_write_linux(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyResult<()> { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 83b199bc4..4ce587e02 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1299,3 +1299,29 @@ fn test_closes_file_descriptors() { .with_limit(Resource::NOFILE, 9, 9) .succeeds(); } + +#[test] +fn test_copy_dir_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir"); + at.symlink_dir("dir", "dir-link"); + ucmd.args(&["-r", "dir-link", "copy"]).succeeds(); + assert_eq!(at.resolve_link("copy"), "dir"); +} + +#[test] +fn test_copy_dir_with_symlinks() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir"); + at.make_file("dir/file"); + + TestScenario::new("ln") + .ucmd() + .arg("-sr") + .arg(at.subdir.join("dir/file")) + .arg(at.subdir.join("dir/file-link")) + .succeeds(); + + ucmd.args(&["-r", "dir", "copy"]).succeeds(); + assert_eq!(at.resolve_link("copy/file-link"), "file"); +} From 315bfd65a3f07221abdb262ba8731d364441e581 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 18 Jun 2021 11:42:37 +0200 Subject: [PATCH 2/4] cp: move symlink check to the right place --- src/uu/cp/src/cp.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 7e7bcca4c..8f47adc28 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -895,14 +895,7 @@ fn copy_source( options: &Options, ) -> CopyResult<()> { let source_path = Path::new(&source); - // if no-dereference is enabled and this is a symlink, don't treat it as a directory - if source_path.is_dir() - && !(!options.dereference - && fs::symlink_metadata(source_path) - .unwrap() - .file_type() - .is_symlink()) - { + if source_path.is_dir() { // Copy as directory copy_directory(source, target, options) } else { @@ -943,6 +936,11 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR return Err(format!("omitting directory '{}'", root.display()).into()); } + // if no-dereference is enabled and this is a symlink, copy it as a file + if !options.dereference && fs::symlink_metadata(root).unwrap().file_type().is_symlink() { + return copy_file(root, target, options); + } + let root_path = env::current_dir().unwrap().join(root); let root_parent = if target.exists() { From 32526e30486613b3e83840331856ae08177dff7a Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 18 Jun 2021 11:45:04 +0200 Subject: [PATCH 3/4] cp: one more clippy fix --- src/uu/cp/src/cp.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 8f47adc28..c1d5e0ee3 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1397,9 +1397,9 @@ pub fn paths_refer_to_same_file(p1: &Path, p2: &Path) -> io::Result { fn test_cp_localize_to_target() { assert!( localize_to_target( - &Path::new("a/source/"), - &Path::new("a/source/c.txt"), - &Path::new("target/") + Path::new("a/source/"), + Path::new("a/source/c.txt"), + Path::new("target/") ) .unwrap() == Path::new("target/c.txt") From a371c034311b17a66460605a786565307791ac46 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 18 Jun 2021 11:48:13 +0200 Subject: [PATCH 4/4] cp: only get the current directory once --- src/uu/cp/src/cp.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index c1d5e0ee3..a7694ae08 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -941,7 +941,10 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR return copy_file(root, target, options); } - let root_path = env::current_dir().unwrap().join(root); + let current_dir = + env::current_dir().unwrap_or_else(|e| crash!(1, "failed to get current directory {}", e)); + + let root_path = current_dir.join(root); let root_parent = if target.exists() { root_path.parent() @@ -968,10 +971,7 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR { let p = or_continue!(path); let is_symlink = fs::symlink_metadata(p.path())?.file_type().is_symlink(); - let path = match env::current_dir() { - Ok(cwd) => cwd.join(&p.path()), - Err(e) => crash!(1, "failed to get current directory {}", e), - }; + let path = current_dir.join(&p.path()); let local_to_root_parent = match root_parent { Some(parent) => {