From 78a11ad69bfc84d6cf860bd2107991de3619445d Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 3 Jul 2022 13:25:18 -0400 Subject: [PATCH 1/4] tests/common/util: add AtPath::symlink_exists() Add helper method for deciding whether a symbolic link exists in the test directory. --- tests/common/util.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/common/util.rs b/tests/common/util.rs index d601b90d841..97d435875e2 100644 --- a/tests/common/util.rs +++ b/tests/common/util.rs @@ -755,6 +755,14 @@ impl AtPath { } } + /// Decide whether the named symbolic link exists in the test directory. + pub fn symlink_exists(&self, path: &str) -> bool { + match fs::symlink_metadata(&self.plus(path)) { + Ok(m) => m.file_type().is_symlink(), + Err(_) => false, + } + } + pub fn dir_exists(&self, path: &str) -> bool { match fs::metadata(&self.plus(path)) { Ok(m) => m.is_dir(), From a7a9da9672a46cc9333ef54c79097bed4cf858ba Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 3 Jul 2022 13:00:58 -0400 Subject: [PATCH 2/4] cp: refactor convenience function is_symlink() Refactor common code used in several places into a convenience function `is_symlink()` that behaves like `Path::is_symlink()` added in Rust 1.58.0. (We support earlier versions of Rust so we cannot use the standard library version of this function.) --- src/uu/cp/src/cp.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 91cbc635ff5..c38daecf6be 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -983,6 +983,16 @@ fn adjust_canonicalization(p: &Path) -> Cow { } } +/// Decide whether the given path is a symbolic link. +fn is_symlink(path: &Path) -> bool { + // TODO Replace this convenience function with `Path::is_symlink()` + // when the minimum supported version of Rust is 1.58 or greater. + match fs::symlink_metadata(path) { + Err(_) => false, + Ok(m) => m.file_type().is_symlink(), + } +} + /// Read the contents of the directory `root` and recursively copy the /// contents to `target`. /// @@ -999,9 +1009,7 @@ fn copy_directory( } // 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() - // replace by is_symlink in rust>=1.58 - { + if !options.dereference && is_symlink(root) { return copy_file(root, target, options, symlinked_files); } @@ -1044,8 +1052,6 @@ fn copy_directory( .follow_links(options.dereference) { let p = or_continue!(path); - let is_symlink = fs::symlink_metadata(p.path())?.file_type().is_symlink(); - // replace by is_symlink in rust >=1.58 let path = current_dir.join(&p.path()); let local_to_root_parent = match root_parent { @@ -1069,7 +1075,7 @@ fn copy_directory( }; let local_to_target = target.join(&local_to_root_parent); - if is_symlink && !options.dereference { + if is_symlink(&path) && !options.dereference { copy_link(&path, &local_to_target, symlinked_files)?; } else if path.is_dir() && !local_to_target.exists() { if target.is_file() { @@ -1091,7 +1097,7 @@ fn copy_directory( ) { Ok(_) => Ok(()), Err(err) => { - if fs::symlink_metadata(&source)?.file_type().is_symlink() { + if is_symlink(&source) { // silent the error with a symlink // In case we do --archive, we might copy the symlink // before the file itself @@ -1306,10 +1312,7 @@ fn copy_file( } // Fail if dest is a dangling symlink or a symlink this program created previously - if fs::symlink_metadata(dest) - .map(|m| m.file_type().is_symlink()) // replace by is_symlink in rust>=1.58 - .unwrap_or(false) - { + if is_symlink(dest) { if FileInformation::from_path(dest, false) .map(|info| symlinked_files.contains(&info)) .unwrap_or(false) @@ -1351,9 +1354,7 @@ fn copy_file( #[cfg(not(unix))] let source_is_fifo = false; - let dest_already_exists_as_symlink = fs::symlink_metadata(&dest) - .map(|meta| meta.file_type().is_symlink()) - .unwrap_or(false); + let dest_already_exists_as_symlink = is_symlink(dest); let dest = if !(source_is_symlink && dest_already_exists_as_symlink) { canonicalize(dest, MissingHandling::Missing, ResolveMode::Physical).unwrap() @@ -1457,10 +1458,7 @@ fn copy_file( }; // TODO: implement something similar to gnu's lchown - if fs::symlink_metadata(&dest) - .map(|meta| !meta.file_type().is_symlink()) - .unwrap_or(false) - { + if !is_symlink(&dest) { // Here, to match GNU semantics, we quietly ignore an error // if a user does not have the correct ownership to modify // the permissions of a file. From 4780690caa845ab144aa69875e0aaff6e90cdf32 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 3 Jul 2022 13:01:23 -0400 Subject: [PATCH 3/4] cp: copy attributes of dangling symbolic link Fix a bug in which `cp` incorrectly exited with an error when attempting to copy the attributes of a dangling symbolic link (that is, when running `cp -P -p`). Fixes #3531. --- src/uu/cp/src/cp.rs | 31 ++++++++++++++++++++----------- tests/by-util/test_cp.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index c38daecf6be..3a9f06ac6c7 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1146,12 +1146,19 @@ fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResu let source_metadata = fs::symlink_metadata(source).context(context)?; match *attribute { Attribute::Mode => { - fs::set_permissions(dest, source_metadata.permissions()).context(context)?; - // FIXME: Implement this for windows as well - #[cfg(feature = "feat_acl")] - exacl::getfacl(source, None) - .and_then(|acl| exacl::setfacl(&[dest], &acl, None)) - .map_err(|err| Error::Error(err.to_string()))?; + // The `chmod()` system call that underlies the + // `fs::set_permissions()` call is unable to change the + // permissions of a symbolic link. In that case, we just + // do nothing, since every symbolic link has the same + // permissions. + if !is_symlink(dest) { + fs::set_permissions(dest, source_metadata.permissions()).context(context)?; + // FIXME: Implement this for windows as well + #[cfg(feature = "feat_acl")] + exacl::getfacl(source, None) + .and_then(|acl| exacl::setfacl(&[dest], &acl, None)) + .map_err(|err| Error::Error(err.to_string()))?; + } } #[cfg(unix)] Attribute::Ownership => { @@ -1177,11 +1184,13 @@ fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResu .map_err(Error::Error)?; } Attribute::Timestamps => { - filetime::set_file_times( - Path::new(dest), - FileTime::from_last_access_time(&source_metadata), - FileTime::from_last_modification_time(&source_metadata), - )?; + let atime = FileTime::from_last_access_time(&source_metadata); + let mtime = FileTime::from_last_modification_time(&source_metadata); + if is_symlink(dest) { + filetime::set_symlink_file_times(dest, atime, mtime)?; + } else { + filetime::set_file_times(dest, atime, mtime)?; + } } #[cfg(feature = "feat_selinux")] Attribute::Context => { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c246d0ef93a..098d73178a8 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -9,6 +9,8 @@ use std::os::unix::fs; #[cfg(unix)] use std::os::unix::fs::symlink as symlink_file; +#[cfg(unix)] +use std::os::unix::fs::MetadataExt; #[cfg(all(unix, not(target_os = "freebsd")))] use std::os::unix::fs::PermissionsExt; #[cfg(windows)] @@ -1536,6 +1538,37 @@ fn test_copy_through_dangling_symlink_no_dereference() { .no_stdout(); } +/// Test for copying a dangling symbolic link and its permissions. +#[test] +fn test_copy_through_dangling_symlink_no_dereference_permissions() { + let (at, mut ucmd) = at_and_ucmd!(); + // target name link name + at.symlink_file("no-such-file", "dangle"); + // don't dereference the link + // | copy permissions, too + // | | from the link + // | | | to new file d2 + // | | | | + // V V V V + ucmd.args(&["-P", "-p", "dangle", "d2"]) + .succeeds() + .no_stderr() + .no_stdout(); + assert!(at.symlink_exists("d2")); + + // `-p` means `--preserve=mode,ownership,timestamps` + #[cfg(unix)] + { + let metadata1 = at.symlink_metadata("dangle"); + let metadata2 = at.symlink_metadata("d2"); + assert_eq!(metadata1.mode(), metadata2.mode()); + assert_eq!(metadata1.uid(), metadata2.uid()); + assert_eq!(metadata1.atime(), metadata2.atime()); + assert_eq!(metadata1.mtime(), metadata2.mtime()); + assert_eq!(metadata1.ctime(), metadata2.ctime()); + } +} + #[test] #[cfg(unix)] fn test_cp_archive_on_nonexistent_file() { From 450bd3b597c3bd6b80931881891112837805daf2 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 6 Jul 2022 11:18:31 +0200 Subject: [PATCH 4/4] Remove the is_symlink function --- src/uu/cp/src/cp.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 9f93d8f64c7..89f534940d9 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -983,16 +983,6 @@ fn adjust_canonicalization(p: &Path) -> Cow { } } -/// Decide whether the given path is a symbolic link. -fn is_symlink(path: &Path) -> bool { - // TODO Replace this convenience function with `Path::is_symlink()` - // when the minimum supported version of Rust is 1.58 or greater. - match fs::symlink_metadata(path) { - Err(_) => false, - Ok(m) => m.file_type().is_symlink(), - } -} - /// Read the contents of the directory `root` and recursively copy the /// contents to `target`. ///