From 8b74562820394359a38f849e291987176e0dc063 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Mon, 30 Aug 2021 01:27:47 +0200 Subject: [PATCH 1/3] cp: correctly copy mode, ownership, acl and context Fix a mix-up between ownership and mode. The latter (mode / file permissions) can also be set on windows (which however only affects the read-only flag), while there doesn't seem to be a straight-forward way to change file ownership on windows. Copy the acl as well when copying the mode. This is a non-default feature and can be enabled with --features feat_acl, because it doesn't seem to work on CI. It is only available for unix so far. Copy the SELinux context if possible. --- .../cspell.dictionaries/jargon.wordlist.txt | 3 + .../workspace.wordlist.txt | 1 + Cargo.lock | 112 +++++++++++- Cargo.toml | 7 +- src/uu/cp/Cargo.toml | 8 +- src/uu/cp/src/cp.rs | 171 +++++++++++++----- src/uucore/Cargo.toml | 2 +- tests/by-util/test_cp.rs | 50 ++++- 8 files changed, 303 insertions(+), 51 deletions(-) diff --git a/.vscode/cspell.dictionaries/jargon.wordlist.txt b/.vscode/cspell.dictionaries/jargon.wordlist.txt index 34abfc511..516d5b4c5 100644 --- a/.vscode/cspell.dictionaries/jargon.wordlist.txt +++ b/.vscode/cspell.dictionaries/jargon.wordlist.txt @@ -35,6 +35,7 @@ falsey fileio flamegraph fullblock +getfacl gibi gibibytes glob @@ -49,6 +50,7 @@ iflag iflags kibi kibibytes +libacl lcase lossily mebi @@ -91,6 +93,7 @@ seedable semver semiprime semiprimes +setfacl shortcode shortcodes siginfo diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 82cbbe15f..29957fb12 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -16,6 +16,7 @@ chrono conv corasick crossterm +exacl filetime formatteriteminfo fsext diff --git a/Cargo.lock b/Cargo.lock index c632db295..514bdf45a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -650,6 +650,17 @@ dependencies = [ "syn", ] +[[package]] +name = "derivative" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fcc3dd5e9e9c0b295d6e1e4d811fb6f157d5ffd784b8d202fc62eac8035a770b" +dependencies = [ + "proc-macro2", + "quote 1.0.9", + "syn", +] + [[package]] name = "diff" version = "0.1.12" @@ -721,6 +732,21 @@ dependencies = [ "termcolor", ] +[[package]] +name = "exacl" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "769bbd173781e84865b957cf83449f0d2869f4c9d2f191cbbffffb3d9751ba2b" +dependencies = [ + "bitflags", + "log", + "nix 0.21.0", + "num_enum", + "scopeguard", + "serde", + "uuid", +] + [[package]] name = "fake-simd" version = "0.1.2" @@ -964,9 +990,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "libc" -version = "0.2.85" +version = "0.2.101" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7ccac4b00700875e6a07c6cde370d44d32fa01c5a65cdd2fca6858c479d28bb3" +checksum = "3cb00336871be5ed2c8ed44b60ae9959dc5b9f08539422ed43f09e34ecaeba21" [[package]] name = "libloading" @@ -1115,6 +1141,19 @@ dependencies = [ "libc", ] +[[package]] +name = "nix" +version = "0.21.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c3728fec49d363a50a8828a190b379a446cc5cf085c06259bbbeb34447e4ec7" +dependencies = [ + "bitflags", + "cc", + "cfg-if 1.0.0", + "libc", + "memoffset", +] + [[package]] name = "nodrop" version = "0.1.14" @@ -1182,6 +1221,28 @@ dependencies = [ "libc", ] +[[package]] +name = "num_enum" +version = "0.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9bd055fb730c4f8f4f57d45d35cd6b3f0980535b056dc7ff119cee6a66ed6f" +dependencies = [ + "derivative", + "num_enum_derive", +] + +[[package]] +name = "num_enum_derive" +version = "0.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "486ea01961c4a818096de679a8b740b26d9033146ac5291b1c98557658f8cdd9" +dependencies = [ + "proc-macro-crate", + "proc-macro2", + "quote 1.0.9", + "syn", +] + [[package]] name = "number_prefix" version = "0.4.0" @@ -1349,6 +1410,16 @@ dependencies = [ "output_vt100", ] +[[package]] +name = "proc-macro-crate" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41fdbd1df62156fbc5945f4762632564d7d038153091c3fcf1067f6aef7cff92" +dependencies = [ + "thiserror", + "toml", +] + [[package]] name = "proc-macro-error" version = "1.0.4" @@ -1706,6 +1777,26 @@ dependencies = [ "walkdir", ] +[[package]] +name = "serde" +version = "1.0.130" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f12d06de37cf59146fbdecab66aa99f9fe4f78722e3607577a5375d66bd0c913" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.130" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d7bc1a1ab1961464eae040d96713baa5a724a8152c1222492465b54322ec508b" +dependencies = [ + "proc-macro2", + "quote 1.0.9", + "syn", +] + [[package]] name = "sha1" version = "0.6.0" @@ -1981,6 +2072,15 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "toml" +version = "0.5.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a31142970826733df8241ef35dc040ef98c679ab14d7c3e54d827099b3acecaa" +dependencies = [ + "serde", +] + [[package]] name = "typenum" version = "1.13.0" @@ -2196,10 +2296,12 @@ name = "uu_cp" version = "0.0.7" dependencies = [ "clap", + "exacl", "filetime", "ioctl-sys", "libc", "quick-error 1.2.3", + "selinux", "uucore", "uucore_procs", "walkdir", @@ -3192,6 +3294,12 @@ dependencies = [ "syn", ] +[[package]] +name = "uuid" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc5cf98d8186244414c848017f0e2676b3fcb46807f6668a97dfe67359a3c4b7" + [[package]] name = "vec_map" version = "0.8.2" diff --git a/Cargo.toml b/Cargo.toml index 84e1494de..3a2c5f12a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -147,7 +147,12 @@ feat_os_unix_musl = [ # NOTE: # The selinux(-sys) crate requires `libselinux` headers and shared library to be accessible in the C toolchain at compile time. # Running a uutils compiled with `feat_selinux` requires an SELinux enabled Kernel at run time. -feat_selinux = ["id/selinux", "selinux", "feat_require_selinux"] +feat_selinux = ["cp/selinux", "id/selinux", "selinux", "feat_require_selinux"] +# "feat_acl" == set of utilities providing support for acl (access control lists) if enabled with `--features feat_acl`. +# NOTE: +# On linux, the posix-acl/acl-sys crate requires `libacl` headers and shared library to be accessible in the C toolchain at compile time. +# On FreeBSD and macOS this is not required. +feat_acl = ["cp/feat_acl"] ## feature sets with requirements (restricting cross-platform availability) # # ** NOTE: these `feat_require_...` sets should be minimized as much as possible to encourage cross-platform availability of utilities diff --git a/src/uu/cp/Cargo.toml b/src/uu/cp/Cargo.toml index ca292c9a1..690a01425 100644 --- a/src/uu/cp/Cargo.toml +++ b/src/uu/cp/Cargo.toml @@ -23,7 +23,8 @@ clap = { version = "2.33", features = ["wrap_help"] } filetime = "0.2" libc = "0.2.85" quick-error = "1.2.3" -uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["fs"] } +selinux = { version="0.2.3", optional=true } +uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["fs", "perms"] } uucore_procs = { version=">=0.0.6", package="uucore_procs", path="../../uucore_procs" } walkdir = "2.2" @@ -35,7 +36,12 @@ winapi = { version="0.3", features=["fileapi"] } [target.'cfg(unix)'.dependencies] xattr="0.2.1" +exacl= { version = "0.6.0", optional=true } [[bin]] name = "cp" path = "src/main.rs" + +[features] +feat_selinux = ["selinux"] +feat_acl = ["exacl"] diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index df70ccea8..772e104ef 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -18,6 +18,7 @@ extern crate quick_error; #[macro_use] extern crate uucore; +use quick_error::Context; #[cfg(windows)] use winapi::um::fileapi::CreateFileW; #[cfg(windows)] @@ -67,6 +68,7 @@ quick_error! { IoErrContext(err: io::Error, path: String) { display("{}: {}", path, err) context(path: &'a str, err: io::Error) -> (err, path.to_owned()) + context(context: String, err: io::Error) -> (err, context) cause(err) } @@ -180,12 +182,15 @@ pub enum CopyMode { AttrOnly, } -#[derive(Clone, Eq, PartialEq)] +// The ordering here determines the order in which attributes are (re-)applied. +// In particular, Ownership must be changed first to avoid interfering with mode change. +#[derive(Clone, Eq, PartialEq, Debug, PartialOrd, Ord)] pub enum Attribute { #[cfg(unix)] - Mode, Ownership, + Mode, Timestamps, + #[cfg(feature = "feat_selinux")] Context, Links, Xattr, @@ -240,7 +245,7 @@ mod options { pub const LINK: &str = "link"; pub const NO_CLOBBER: &str = "no-clobber"; pub const NO_DEREFERENCE: &str = "no-dereference"; - pub const NO_DEREFERENCE_PRESERVE_LINKS: &str = "no-dereference-preserve-linkgs"; + pub const NO_DEREFERENCE_PRESERVE_LINKS: &str = "no-dereference-preserve-links"; pub const NO_PRESERVE: &str = "no-preserve"; pub const NO_TARGET_DIRECTORY: &str = "no-target-directory"; pub const ONE_FILE_SYSTEM: &str = "one-file-system"; @@ -266,6 +271,7 @@ static PRESERVABLE_ATTRIBUTES: &[&str] = &[ "mode", "ownership", "timestamps", + #[cfg(feature = "feat_selinux")] "context", "links", "xattr", @@ -273,18 +279,12 @@ static PRESERVABLE_ATTRIBUTES: &[&str] = &[ ]; #[cfg(not(unix))] -static PRESERVABLE_ATTRIBUTES: &[&str] = &[ - "ownership", - "timestamps", - "context", - "links", - "xattr", - "all", -]; +static PRESERVABLE_ATTRIBUTES: &[&str] = + &["mode", "timestamps", "context", "links", "xattr", "all"]; static DEFAULT_ATTRIBUTES: &[Attribute] = &[ - #[cfg(unix)] Attribute::Mode, + #[cfg(unix)] Attribute::Ownership, Attribute::Timestamps, ]; @@ -381,13 +381,13 @@ pub fn uu_app() -> App<'static, 'static> { .conflicts_with_all(&[options::PRESERVE_DEFAULT_ATTRIBUTES, options::NO_PRESERVE]) // -d sets this option // --archive sets this option - .help("Preserve the specified attributes (default: mode (unix only), ownership, timestamps), \ + .help("Preserve the specified attributes (default: mode, ownership (unix only), timestamps), \ if possible additional attributes: context, links, xattr, all")) .arg(Arg::with_name(options::PRESERVE_DEFAULT_ATTRIBUTES) .short("-p") .long(options::PRESERVE_DEFAULT_ATTRIBUTES) .conflicts_with_all(&[options::PRESERVE, options::NO_PRESERVE, options::ARCHIVE]) - .help("same as --preserve=mode(unix only),ownership,timestamps")) + .help("same as --preserve=mode,ownership(unix only),timestamps")) .arg(Arg::with_name(options::NO_PRESERVE) .long(options::NO_PRESERVE) .takes_value(true) @@ -532,10 +532,11 @@ impl FromStr for Attribute { fn from_str(value: &str) -> CopyResult { Ok(match &*value.to_lowercase() { - #[cfg(unix)] "mode" => Attribute::Mode, + #[cfg(unix)] "ownership" => Attribute::Ownership, "timestamps" => Attribute::Timestamps, + #[cfg(feature = "feat_selinux")] "context" => Attribute::Context, "links" => Attribute::Links, "xattr" => Attribute::Xattr, @@ -552,14 +553,16 @@ impl FromStr for Attribute { fn add_all_attributes() -> Vec { use Attribute::*; - #[cfg(target_os = "windows")] - let attr = vec![Ownership, Timestamps, Context, Xattr, Links]; - - #[cfg(not(target_os = "windows"))] - let mut attr = vec![Ownership, Timestamps, Context, Xattr, Links]; - - #[cfg(unix)] - attr.insert(0, Mode); + let attr = vec![ + #[cfg(unix)] + Ownership, + Mode, + Timestamps, + #[cfg(feature = "feat_selinux")] + Context, + Links, + Xattr, + ]; attr } @@ -602,7 +605,7 @@ impl Options { .map(ToString::to_string); // Parse attributes to preserve - let preserve_attributes: Vec = if matches.is_present(options::PRESERVE) { + let mut preserve_attributes: Vec = if matches.is_present(options::PRESERVE) { match matches.values_of(options::PRESERVE) { None => DEFAULT_ATTRIBUTES.to_vec(), Some(attribute_strs) => { @@ -629,6 +632,11 @@ impl Options { vec![] }; + // Make sure ownership is changed before other attributes, + // as chown clears some of the permission and therefore could undo previous changes + // if not executed first. + preserve_attributes.sort_unstable(); + let options = Options { attributes_only: matches.is_present(options::ATTRIBUTES_ONLY), copy_contents: matches.is_present(options::COPY_CONTENTS), @@ -1048,28 +1056,79 @@ impl OverwriteMode { } } -fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResult<()> { +fn copy_attribute( + source: &Path, + dest: &Path, + attribute: &Attribute, + options: &Options, +) -> CopyResult<()> { let context = &*format!("'{}' -> '{}'", source.display().to_string(), dest.display()); + let source_metadata = if options.dereference { + source.metadata() + } else { + source.symlink_metadata() + } + .context(context)?; match *attribute { - #[cfg(unix)] Attribute::Mode => { - let mode = fs::metadata(source).context(context)?.permissions().mode(); - let mut dest_metadata = fs::metadata(source).context(context)?.permissions(); - dest_metadata.set_mode(mode); + fs::set_permissions(dest, source_metadata.permissions()).context(context)?; + dest.metadata().unwrap().permissions(); + // 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 => { - let metadata = fs::metadata(source).context(context)?; - fs::set_permissions(dest, metadata.permissions()).context(context)?; + use std::os::unix::prelude::MetadataExt; + use uucore::perms::wrap_chown; + use uucore::perms::Verbosity; + use uucore::perms::VerbosityLevel; + + let dest_uid = source_metadata.uid(); + let dest_gid = source_metadata.gid(); + + wrap_chown( + dest, + &dest.metadata().context(context)?, + Some(dest_uid), + Some(dest_gid), + false, + Verbosity { + groups_only: false, + level: VerbosityLevel::Normal, + }, + ) + .map_err(Error::Error)?; } Attribute::Timestamps => { - let metadata = fs::metadata(source)?; filetime::set_file_times( Path::new(dest), - FileTime::from_last_access_time(&metadata), - FileTime::from_last_modification_time(&metadata), + FileTime::from_last_access_time(&source_metadata), + FileTime::from_last_modification_time(&source_metadata), )?; } - Attribute::Context => {} + #[cfg(feature = "feat_selinux")] + Attribute::Context => { + let context = selinux::SecurityContext::of_path(source, options.dereference, false) + .map_err(|e| { + format!( + "failed to get security context of {}: {}", + source.display(), + e + ) + })?; + if let Some(context) = context { + context.set_for_path(dest, false, false).map_err(|e| { + format!( + "failed to set security context for {}: {}", + dest.display(), + e + ) + })?; + } + } Attribute::Links => {} Attribute::Xattr => { #[cfg(unix)] @@ -1087,6 +1146,7 @@ fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResu } } }; + dest.metadata().unwrap().permissions(); Ok(()) } @@ -1160,17 +1220,37 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { if options.verbose { println!("{}", context_for(source, dest)); } + // FIXME: `source` and `dest` should be dereferenced here if appropriate, so that the rest + // of the code does not have to check `options.dereference` all the time and can work with the paths directly. - #[allow(unused)] - { - // TODO: implement --preserve flag - let mut preserve_context = false; - for attribute in &options.preserve_attributes { - if *attribute == Attribute::Context { - preserve_context = true; - } + let dest_permissions = if dest.exists() { + dest.metadata() + .map_err(|e| Context(context_for(dest, source), e))? + .permissions() + } else { + #[allow(unused_mut)] + let mut permissions = source + .metadata() + .map_err(|e| Context(context_for(dest, source), e))? + .permissions(); + #[cfg(unix)] + { + use uucore::mode::get_umask; + + let mut mode = permissions.mode(); + + // remove sticky bit, suid and gid bit + const SPECIAL_PERMS_MASK: u32 = 0o7000; + mode &= !SPECIAL_PERMS_MASK; + + // apply umask + mode &= !get_umask(); + + permissions.set_mode(mode); } - } + permissions + }; + match options.copy_mode { CopyMode::Link => { fs::hard_link(source, dest).context(&*context_for(source, dest))?; @@ -1207,8 +1287,9 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { .unwrap(); } }; + fs::set_permissions(dest, dest_permissions).unwrap(); for attribute in &options.preserve_attributes { - copy_attribute(source, dest, attribute)?; + copy_attribute(source, dest, attribute, options)?; } Ok(()) } diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index e45c9e5b5..fd78e6551 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -29,7 +29,7 @@ walkdir = { version="2.3.2", optional=true } data-encoding = { version="2.1", optional=true } data-encoding-macro = { version="0.1.12", optional=true } z85 = { version="3.0.3", optional=true } -libc = { version="0.2.15, <= 0.2.85", optional=true } ## libc: initial utmp support added in v0.2.15; but v0.2.68 breaks the build for MinSRV v1.31.0 +libc = { version="0.2.15", optional=true } [dev-dependencies] clap = "2.33.3" diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index b6b4c3311..c77f4c01d 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7,7 +7,7 @@ use std::fs::set_permissions; #[cfg(not(windows))] use std::os::unix::fs; -#[cfg(target_os = "linux")] +#[cfg(unix)] use std::os::unix::fs::PermissionsExt; #[cfg(windows)] use std::os::windows::fs::symlink_file; @@ -1305,3 +1305,51 @@ fn test_copy_symlink_force() { .succeeds(); assert_eq!(at.resolve_link("copy"), "file"); } + +#[test] +#[cfg(unix)] +fn test_no_preserve_mode() { + use std::os::unix::prelude::MetadataExt; + + use uucore::mode::get_umask; + + const PERMS_ALL: u32 = 0o7777; + + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file"); + set_permissions(at.plus("file"), PermissionsExt::from_mode(PERMS_ALL)).unwrap(); + ucmd.arg("file") + .arg("dest") + .succeeds() + .no_stderr() + .no_stdout(); + let umask = get_umask(); + // remove sticky bit, setuid and setgid bit; apply umask + let expected_perms = PERMS_ALL & !0o7000 & !umask; + assert_eq!( + at.plus("dest").metadata().unwrap().mode() & 0o7777, + expected_perms + ); +} + +#[test] +#[cfg(unix)] +fn test_preserve_mode() { + use std::os::unix::prelude::MetadataExt; + + const PERMS_ALL: u32 = 0o7777; + + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file"); + set_permissions(at.plus("file"), PermissionsExt::from_mode(PERMS_ALL)).unwrap(); + ucmd.arg("file") + .arg("dest") + .arg("-p") + .succeeds() + .no_stderr() + .no_stdout(); + assert_eq!( + at.plus("dest").metadata().unwrap().mode() & 0o7777, + PERMS_ALL + ); +} From ef9c5d4fcf13fdc0c7bc8f3a8d14f84c3986ee51 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Mon, 30 Aug 2021 22:16:36 +0200 Subject: [PATCH 2/3] cp: canonicalize paths upfront This way later code can assume `src` and `dest` to be the actual paths of source and destination, and do not have to constantly check `options.dereference`. This requires moving the error context calculation to the beginning as well, since later steps no longer operate with the same file paths as supplied by the user. --- src/uu/cp/Cargo.toml | 2 +- src/uu/cp/src/cp.rs | 136 +++++++++++++++++++++---------------------- 2 files changed, 68 insertions(+), 70 deletions(-) diff --git a/src/uu/cp/Cargo.toml b/src/uu/cp/Cargo.toml index 690a01425..66beb2501 100644 --- a/src/uu/cp/Cargo.toml +++ b/src/uu/cp/Cargo.toml @@ -24,7 +24,7 @@ filetime = "0.2" libc = "0.2.85" quick-error = "1.2.3" selinux = { version="0.2.3", optional=true } -uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["fs", "perms"] } +uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["fs", "perms", "mode"] } uucore_procs = { version=">=0.0.6", package="uucore_procs", path="../../uucore_procs" } walkdir = "2.2" diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 772e104ef..e9e76237b 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -18,7 +18,6 @@ extern crate quick_error; #[macro_use] extern crate uucore; -use quick_error::Context; #[cfg(windows)] use winapi::um::fileapi::CreateFileW; #[cfg(windows)] @@ -1056,23 +1055,12 @@ impl OverwriteMode { } } -fn copy_attribute( - source: &Path, - dest: &Path, - attribute: &Attribute, - options: &Options, -) -> CopyResult<()> { +fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResult<()> { let context = &*format!("'{}' -> '{}'", source.display().to_string(), dest.display()); - let source_metadata = if options.dereference { - source.metadata() - } else { - source.symlink_metadata() - } - .context(context)?; + let source_metadata = fs::symlink_metadata(source).context(context)?; match *attribute { Attribute::Mode => { fs::set_permissions(dest, source_metadata.permissions()).context(context)?; - dest.metadata().unwrap().permissions(); // FIXME: Implement this for windows as well #[cfg(feature = "feat_acl")] exacl::getfacl(source, None) @@ -1091,7 +1079,7 @@ fn copy_attribute( wrap_chown( dest, - &dest.metadata().context(context)?, + &dest.symlink_metadata().context(context)?, Some(dest_uid), Some(dest_gid), false, @@ -1111,14 +1099,13 @@ fn copy_attribute( } #[cfg(feature = "feat_selinux")] Attribute::Context => { - let context = selinux::SecurityContext::of_path(source, options.dereference, false) - .map_err(|e| { - format!( - "failed to get security context of {}: {}", - source.display(), - e - ) - })?; + let context = selinux::SecurityContext::of_path(source, false, false).map_err(|e| { + format!( + "failed to get security context of {}: {}", + source.display(), + e + ) + })?; if let Some(context) = context { context.set_for_path(dest, false, false).map_err(|e| { format!( @@ -1146,7 +1133,6 @@ fn copy_attribute( } } }; - dest.metadata().unwrap().permissions(); Ok(()) } @@ -1203,8 +1189,8 @@ fn handle_existing_dest(source: &Path, dest: &Path, options: &Options) -> CopyRe Ok(()) } -/// Copy the a file from `source` to `dest`. No path manipulation is -/// done on either `source` or `dest`, the are used as provided. +/// Copy the a file from `source` to `dest`. `source` will be dereferenced if +/// `options.dereference` is set to true. `dest` will always be dereferenced. /// /// Behavior when copying to existing files is contingent on the /// `options.overwrite` mode. If a file is skipped, the return type @@ -1220,19 +1206,24 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { if options.verbose { println!("{}", context_for(source, dest)); } - // FIXME: `source` and `dest` should be dereferenced here if appropriate, so that the rest - // of the code does not have to check `options.dereference` all the time and can work with the paths directly. + + // Calculate the context upfront before canonicalizing the path + let context = context_for(source, dest); + let context = context.as_str(); + + // canonicalize dest and source so that later steps can work with the paths directly + let dest = canonicalize(dest, MissingHandling::Missing, ResolveMode::Physical).unwrap(); + let source = if options.dereference { + canonicalize(source, MissingHandling::Missing, ResolveMode::Physical).unwrap() + } else { + source.to_owned() + }; let dest_permissions = if dest.exists() { - dest.metadata() - .map_err(|e| Context(context_for(dest, source), e))? - .permissions() + dest.symlink_metadata().context(context)?.permissions() } else { #[allow(unused_mut)] - let mut permissions = source - .metadata() - .map_err(|e| Context(context_for(dest, source), e))? - .permissions(); + let mut permissions = source.symlink_metadata().context(context)?.permissions(); #[cfg(unix)] { use uucore::mode::get_umask; @@ -1253,29 +1244,29 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { match options.copy_mode { CopyMode::Link => { - fs::hard_link(source, dest).context(&*context_for(source, dest))?; + fs::hard_link(&source, &dest).context(context)?; } CopyMode::Copy => { - copy_helper(source, dest, options)?; + copy_helper(&source, &dest, options, context)?; } CopyMode::SymLink => { - symlink_file(source, dest, &*context_for(source, dest))?; + symlink_file(&source, &dest, context)?; } CopyMode::Sparse => return Err(Error::NotImplemented(options::SPARSE.to_string())), CopyMode::Update => { if dest.exists() { - let src_metadata = fs::metadata(source)?; - let dest_metadata = fs::metadata(dest)?; + let src_metadata = fs::symlink_metadata(&source)?; + let dest_metadata = fs::symlink_metadata(&dest)?; let src_time = src_metadata.modified()?; let dest_time = dest_metadata.modified()?; if src_time <= dest_time { return Ok(()); } else { - copy_helper(source, dest, options)?; + copy_helper(&source, &dest, options, context)?; } } else { - copy_helper(source, dest, options)?; + copy_helper(&source, &dest, options, context)?; } } CopyMode::AttrOnly => { @@ -1283,54 +1274,51 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { .write(true) .truncate(false) .create(true) - .open(dest) + .open(&dest) .unwrap(); } }; - fs::set_permissions(dest, dest_permissions).unwrap(); + + // TODO: implement something similar to gnu's lchown + if fs::symlink_metadata(&dest) + .map(|meta| !meta.file_type().is_symlink()) + .unwrap_or(false) + { + fs::set_permissions(&dest, dest_permissions).unwrap(); + } for attribute in &options.preserve_attributes { - copy_attribute(source, dest, attribute, options)?; + copy_attribute(&source, &dest, attribute)?; } Ok(()) } /// Copy the file from `source` to `dest` either using the normal `fs::copy` or a /// copy-on-write scheme if --reflink is specified and the filesystem supports it. -fn copy_helper(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { +fn copy_helper(source: &Path, dest: &Path, options: &Options, context: &str) -> CopyResult<()> { if options.parents { let parent = dest.parent().unwrap_or(dest); fs::create_dir_all(parent)?; } let is_symlink = fs::symlink_metadata(&source)?.file_type().is_symlink(); - if source.to_string_lossy() == "/dev/null" { + if source.as_os_str() == "/dev/null" { /* workaround a limitation of fs::copy * https://github.com/rust-lang/rust/issues/79390 */ File::create(dest)?; - } else if !options.dereference && is_symlink { + } else if is_symlink { copy_link(source, dest)?; } else if options.reflink_mode != ReflinkMode::Never { #[cfg(not(any(target_os = "linux", target_os = "macos")))] return Err("--reflink is only supported on linux and macOS" .to_string() .into()); - #[cfg(any(target_os = "linux", target_os = "macos"))] - if is_symlink { - assert!(options.dereference); - let real_path = std::fs::read_link(source)?; - #[cfg(target_os = "macos")] - copy_on_write_macos(&real_path, dest, options.reflink_mode)?; - #[cfg(target_os = "linux")] - copy_on_write_linux(&real_path, dest, options.reflink_mode)?; - } else { - #[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)?; - } + #[cfg(target_os = "macos")] + copy_on_write_macos(source, dest, options.reflink_mode, context)?; + #[cfg(target_os = "linux")] + copy_on_write_linux(source, dest, options.reflink_mode, context)?; } else { - fs::copy(source, dest).context(&*context_for(source, dest))?; + fs::copy(source, dest).context(context)?; } Ok(()) @@ -1361,16 +1349,21 @@ fn copy_link(source: &Path, dest: &Path) -> CopyResult<()> { /// 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<()> { +fn copy_on_write_linux( + source: &Path, + dest: &Path, + mode: ReflinkMode, + context: &str, +) -> CopyResult<()> { debug_assert!(mode != ReflinkMode::Never); - let src_file = File::open(source).context(&*context_for(source, dest))?; + let src_file = File::open(source).context(context)?; let dst_file = OpenOptions::new() .write(true) .truncate(false) .create(true) .open(dest) - .context(&*context_for(source, dest))?; + .context(context)?; match mode { ReflinkMode::Always => unsafe { let result = ficlone(dst_file.as_raw_fd(), src_file.as_raw_fd() as *const i32); @@ -1389,7 +1382,7 @@ fn copy_on_write_linux(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyRes ReflinkMode::Auto => unsafe { let result = ficlone(dst_file.as_raw_fd(), src_file.as_raw_fd() as *const i32); if result != 0 { - fs::copy(source, dest).context(&*context_for(source, dest))?; + fs::copy(source, dest).context(context)?; } Ok(()) }, @@ -1399,7 +1392,12 @@ fn copy_on_write_linux(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyRes /// Copies `source` to `dest` using copy-on-write if possible. #[cfg(target_os = "macos")] -fn copy_on_write_macos(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyResult<()> { +fn copy_on_write_macos( + source: &Path, + dest: &Path, + mode: ReflinkMode, + context: &str, +) -> CopyResult<()> { debug_assert!(mode != ReflinkMode::Never); // Extract paths in a form suitable to be passed to a syscall. @@ -1444,7 +1442,7 @@ fn copy_on_write_macos(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyRes format!("failed to clone {:?} from {:?}: {}", source, dest, error).into(), ) } - ReflinkMode::Auto => fs::copy(source, dest).context(&*context_for(source, dest))?, + ReflinkMode::Auto => fs::copy(source, dest).context(context)?, ReflinkMode::Never => unreachable!(), }; } From 6a6da71403269df83ef41b3d68ccd67c79230656 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 3 Sep 2021 14:56:35 +0200 Subject: [PATCH 3/3] cp: add a test for #2631 --- tests/by-util/test_cp.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c77f4c01d..08a9643ca 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7,6 +7,8 @@ use std::fs::set_permissions; #[cfg(not(windows))] use std::os::unix::fs; +#[cfg(unix)] +use std::os::unix::fs::symlink as symlink_file; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; #[cfg(windows)] @@ -1353,3 +1355,16 @@ fn test_preserve_mode() { PERMS_ALL ); } + +#[test] +fn test_canonicalize_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir"); + at.touch("dir/file"); + symlink_file("../dir/file", at.plus("dir/file-ln")).unwrap(); + ucmd.arg("dir/file-ln") + .arg(".") + .succeeds() + .no_stderr() + .no_stdout(); +}