From 576aa29f0f47b3456c8d72189c35b133cafa2708 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 12 Dec 2020 10:31:12 +0100 Subject: [PATCH] refactor(chmod): move from walker to walkdir, simplify the code and add tests (#1645) --- Cargo.lock | 9 +---- src/uu/chmod/Cargo.toml | 2 +- src/uu/chmod/src/chmod.rs | 77 +++++++++++++------------------------ tests/by-util/test_chmod.rs | 67 ++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2e24be40e..3ce4ad288 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -471,6 +471,7 @@ version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "cfg-if 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", + "const_fn 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", "const_fn 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "crossbeam-utils 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1435,7 +1436,7 @@ dependencies = [ "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)", "uucore 0.0.4", "uucore_procs 0.0.4", - "walker 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", + "walkdir 2.3.1 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -2381,11 +2382,6 @@ dependencies = [ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "walker" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" - [[package]] name = "wasi" version = "0.9.0+wasi-snapshot-preview1" @@ -2664,7 +2660,6 @@ dependencies = [ "checksum vec_map 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)" = "f1bddf1187be692e79c5ffeab891132dfb0f236ed36a43c7ed39f1165ee20191" "checksum void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d" "checksum walkdir 2.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "777182bc735b6424e1a57516d35ed72cb8019d85c8c9bf536dccb3445c1a2f7d" -"checksum walker 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "44971d5e5ae4f7904dffb6260ebd3910e7bcae104a94730e04a24cb6af40646b" "checksum wasi 0.9.0+wasi-snapshot-preview1 (registry+https://github.com/rust-lang/crates.io-index)" = "cccddf32554fecc6acb585f82a32a72e28b48f8c4c1883ddfeeeaa96f7d8e519" "checksum wasm-bindgen 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)" = "3cd364751395ca0f68cafb17666eee36b63077fb5ecd972bbcd74c90c4bf736e" "checksum wasm-bindgen-backend 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)" = "1114f89ab1f4106e5b55e688b828c0ab0ea593a1ea7c094b141b14cbaaec2d62" diff --git a/src/uu/chmod/Cargo.toml b/src/uu/chmod/Cargo.toml index 3c257cd4d..3994fea92 100644 --- a/src/uu/chmod/Cargo.toml +++ b/src/uu/chmod/Cargo.toml @@ -18,7 +18,7 @@ path = "src/chmod.rs" libc = "0.2.42" uucore = { version=">=0.0.4", package="uucore", path="../../uucore", features=["fs", "mode"] } uucore_procs = { version=">=0.0.4", package="uucore_procs", path="../../uucore_procs" } -walker = "1.0.0" +walkdir = "2.2" [[bin]] name = "chmod" diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index ce3c3dd96..bcba604e9 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -9,7 +9,7 @@ #[cfg(unix)] extern crate libc; -extern crate walker; +extern crate walkdir; #[macro_use] extern crate uucore; @@ -20,7 +20,7 @@ use std::path::Path; use uucore::fs::display_permissions_unix; #[cfg(not(windows))] use uucore::mode; -use walker::Walker; +use walkdir::WalkDir; const NAME: &str = "chmod"; static SUMMARY: &str = "Change the mode of each FILE to MODE. @@ -150,62 +150,39 @@ impl Chmoder { for filename in &files { let filename = &filename[..]; let file = Path::new(filename); - if file.exists() { - if file.is_dir() { - if !self.preserve_root || filename != "/" { - if self.recursive { - let walk_dir = match Walker::new(&file) { - Ok(m) => m, - Err(f) => { - crash!(1, "{}", f.to_string()); - } - }; - // XXX: here (and elsewhere) we see that this impl will have issues - // with non-UTF-8 filenames. Using OsString won't fix this because - // on Windows OsStrings cannot be built out of non-UTF-8 chars. One - // possible fix is to use CStrings rather than Strings in the args - // to chmod() and chmod_file(). - r = self - .chmod( - walk_dir - .filter_map(|x| match x { - Ok(o) => match o.path().into_os_string().to_str() { - Some(s) => Some(s.to_owned()), - None => None, - }, - Err(_) => None, - }) - .collect(), - ) - .and(r); - r = self.chmod_file(&file, filename).and(r); - } - } else { - show_error!("could not change permissions of directory '{}'", filename); - r = Err(1); - } - } else { - r = self.chmod_file(&file, filename).and(r); - } - } else { + if !file.exists() { show_error!("no such file or directory '{}'", filename); - r = Err(1); + return Err(1); + } + if self.recursive && self.preserve_root && filename == "/" { + show_error!( + "it is dangerous to operate recursively on '{}'\nuse --no-preserve-root to override this failsafe", + filename + ); + return Err(1); + } + if !self.recursive { + r = self.chmod_file(&file).and(r); + } else { + for entry in WalkDir::new(&filename).into_iter().filter_map(|e| e.ok()) { + let file = entry.path(); + r = self.chmod_file(&file).and(r); + } } } - r } #[cfg(windows)] - fn chmod_file(&self, file: &Path, name: &str) -> Result<(), i32> { + fn chmod_file(&self, file: &Path) -> Result<(), i32> { // chmod is useless on Windows // it doesn't set any permissions at all // instead it just sets the readonly attribute on the file Err(0) } #[cfg(any(unix, target_os = "redox"))] - fn chmod_file(&self, file: &Path, name: &str) -> Result<(), i32> { - let mut fperm = match fs::metadata(name) { + fn chmod_file(&self, file: &Path) -> Result<(), i32> { + let mut fperm = match fs::metadata(file) { Ok(meta) => meta.mode() & 0o7777, Err(err) => { if !self.quiet { @@ -215,7 +192,7 @@ impl Chmoder { } }; match self.fmode { - Some(mode) => self.change_file(fperm, mode, file, name)?, + Some(mode) => self.change_file(fperm, mode, file)?, None => { let cmode_unwrapped = self.cmode.clone().unwrap(); for mode in cmode_unwrapped.split(',') { @@ -228,7 +205,7 @@ impl Chmoder { }; match result { Ok(mode) => { - self.change_file(fperm, mode, file, name)?; + self.change_file(fperm, mode, file)?; fperm = mode; } Err(f) => { @@ -246,7 +223,7 @@ impl Chmoder { } #[cfg(unix)] - fn change_file(&self, fperm: u32, mode: u32, file: &Path, path: &str) -> Result<(), i32> { + fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> { if fperm == mode { if self.verbose && !self.changes { show_info!( @@ -257,9 +234,7 @@ impl Chmoder { ); } Ok(()) - } else if let Err(err) = - fs::set_permissions(Path::new(path), fs::Permissions::from_mode(mode)) - { + } else if let Err(err) = fs::set_permissions(file, fs::Permissions::from_mode(mode)) { if !self.quiet { show_error!("{}", err); } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 612f0860f..1b1a50e0c 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -279,3 +279,70 @@ fn test_chmod_reference_file() { mkfile(&at.plus_as_string(REFERENCE_FILE), REFERENCE_PERMS); run_single_test(&tests[0], at, ucmd); } + +#[test] +fn test_chmod_recursive() { + let _guard = UMASK_MUTEX.lock(); + + let original_umask = unsafe { umask(0) }; + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("a"); + at.mkdir("a/b"); + at.mkdir("a/b/c"); + at.mkdir("z"); + mkfile(&at.plus_as_string("a/a"), 0o100444); + mkfile(&at.plus_as_string("a/b/b"), 0o100444); + mkfile(&at.plus_as_string("a/b/c/c"), 0o100444); + mkfile(&at.plus_as_string("z/y"), 0o100444); + + let result = ucmd + .arg("-R") + .arg("--verbose") + .arg("-r,a+w") + .arg("a") + .arg("z") + .succeeds(); + + assert_eq!(at.metadata("z/y").permissions().mode(), 0o100222); + assert_eq!(at.metadata("a/a").permissions().mode(), 0o100222); + assert_eq!(at.metadata("a/b/b").permissions().mode(), 0o100222); + assert_eq!(at.metadata("a/b/c/c").permissions().mode(), 0o100222); + println!("mode {:o}", at.metadata("a").permissions().mode()); + assert_eq!(at.metadata("a").permissions().mode(), 0o40333); + assert_eq!(at.metadata("z").permissions().mode(), 0o40333); + assert!(result.stderr.contains("to 333 (-wx-wx-wx)")); + assert!(result.stderr.contains("to 222 (-w--w--w-)")); + + unsafe { + umask(original_umask); + } +} + +#[test] +fn test_chmod_non_existing_file() { + let (_at, mut ucmd) = at_and_ucmd!(); + let result = ucmd + .arg("-R") + .arg("--verbose") + .arg("-r,a+w") + .arg("dont-exist") + .fails(); + assert_eq!( + result.stderr, + "chmod: error: no such file or directory 'dont-exist'\n" + ); +} + +#[test] +fn test_chmod_preserve_root() { + let (_at, mut ucmd) = at_and_ucmd!(); + let result = ucmd + .arg("-R") + .arg("--preserve-root") + .arg("755") + .arg("/") + .fails(); + assert!(result + .stderr + .contains("chmod: error: it is dangerous to operate recursively on '/'")); +}