diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 5108ec924..26b3fd85b 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -98,6 +98,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { Some(cmode) }; + if files.is_empty() { + crash!(1, "missing operand"); + } + let chmoder = Chmoder { changes, quiet, @@ -180,6 +184,9 @@ pub fn uu_app() -> App<'static, 'static> { // e.g. "chmod -v -xw -R FILE" -> "chmod -v xw -R FILE" pub fn strip_minus_from_mode(args: &mut Vec) -> bool { for arg in args { + if arg == "--" { + break; + } if arg.starts_with('-') { if let Some(second) = arg.chars().nth(1) { match second { @@ -222,7 +229,7 @@ impl Chmoder { if !self.quiet { show_error!("cannot operate on dangling symlink '{}'", filename); } - } else { + } else if !self.quiet { show_error!("cannot access '{}': No such file or directory", filename); } return Err(1); @@ -253,9 +260,11 @@ impl Chmoder { // instead it just sets the readonly attribute on the file Err(0) } - #[cfg(any(unix, target_os = "redox"))] + #[cfg(unix)] fn chmod_file(&self, file: &Path) -> Result<(), i32> { - let mut fperm = match fs::metadata(file) { + use uucore::mode::get_umask; + + let fperm = match fs::metadata(file) { Ok(meta) => meta.mode() & 0o7777, Err(err) => { if is_symlink(file) { @@ -278,18 +287,30 @@ impl Chmoder { Some(mode) => self.change_file(fperm, mode, file)?, None => { let cmode_unwrapped = self.cmode.clone().unwrap(); + let mut new_mode = fperm; + let mut naively_expected_new_mode = new_mode; for mode in cmode_unwrapped.split(',') { // cmode is guaranteed to be Some in this case let arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; let result = if mode.contains(arr) { - mode::parse_numeric(fperm, mode) + mode::parse_numeric(new_mode, mode, file.is_dir()).map(|v| (v, v)) } else { - mode::parse_symbolic(fperm, mode, file.is_dir()) + mode::parse_symbolic(new_mode, mode, get_umask(), file.is_dir()).map(|m| { + // calculate the new mode as if umask was 0 + let naive_mode = mode::parse_symbolic( + naively_expected_new_mode, + mode, + 0, + file.is_dir(), + ) + .unwrap(); // we know that mode must be valid, so this cannot fail + (m, naive_mode) + }) }; match result { - Ok(mode) => { - self.change_file(fperm, mode, file)?; - fperm = mode; + Ok((mode, naive_mode)) => { + new_mode = mode; + naively_expected_new_mode = naive_mode; } Err(f) => { if !self.quiet { @@ -299,6 +320,17 @@ impl Chmoder { } } } + self.change_file(fperm, new_mode, file)?; + // if a permission would have been removed if umask was 0, but it wasn't because umask was not 0, print an error and fail + if (new_mode & !naively_expected_new_mode) != 0 { + show_error!( + "{}: new permissions are {}, not {}", + file.display(), + display_permissions_unix(new_mode as mode_t, false), + display_permissions_unix(naively_expected_new_mode as mode_t, false) + ); + return Err(1); + } } } @@ -322,8 +354,8 @@ impl Chmoder { show_error!("{}", err); } if self.verbose { - show_error!( - "failed to change mode of file '{}' from {:o} ({}) to {:o} ({})", + println!( + "failed to change mode of file '{}' from {:04o} ({}) to {:04o} ({})", file.display(), fperm, display_permissions_unix(fperm as mode_t, false), @@ -334,8 +366,8 @@ impl Chmoder { Err(1) } else { if self.verbose || self.changes { - show_error!( - "mode of '{}' changed from {:o} ({}) to {:o} ({})", + println!( + "mode of '{}' changed from {:04o} ({}) to {:04o} ({})", file.display(), fperm, display_permissions_unix(fperm as mode_t, false), diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 88912e8a9..b72721d20 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -18,6 +18,7 @@ use filetime::{set_file_times, FileTime}; use uucore::backup_control::{self, BackupMode}; use uucore::entries::{grp2gid, usr2uid}; use uucore::error::{FromIo, UError, UIoError, UResult}; +use uucore::mode::get_umask; use uucore::perms::{wrap_chown, Verbosity, VerbosityLevel}; use libc::{getegid, geteuid}; @@ -359,7 +360,7 @@ fn behavior(matches: &ArgMatches) -> UResult { let specified_mode: Option = if matches.is_present(OPT_MODE) { let x = matches.value_of(OPT_MODE).ok_or(1)?; - Some(mode::parse(x, considering_dir).map_err(|err| { + Some(mode::parse(x, considering_dir, get_umask()).map_err(|err| { show_error!("Invalid mode string: {}", err); 1 })?) diff --git a/src/uu/install/src/mode.rs b/src/uu/install/src/mode.rs index b8d5cd839..fd4cee50e 100644 --- a/src/uu/install/src/mode.rs +++ b/src/uu/install/src/mode.rs @@ -4,14 +4,14 @@ use std::path::Path; use uucore::mode; /// Takes a user-supplied string and tries to parse to u16 mode bitmask. -pub fn parse(mode_string: &str, considering_dir: bool) -> Result { +pub fn parse(mode_string: &str, considering_dir: bool, umask: u32) -> Result { let numbers: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; // Passing 000 as the existing permissions seems to mirror GNU behavior. if mode_string.contains(numbers) { - mode::parse_numeric(0, mode_string) + mode::parse_numeric(0, mode_string, considering_dir) } else { - mode::parse_symbolic(0, mode_string, considering_dir) + mode::parse_symbolic(0, mode_string, umask, considering_dir) } } diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 4cf3cf6f3..02e78ba9b 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -9,59 +9,64 @@ // spell-checker:ignore (vars) fperm srwx -use libc::{mode_t, S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR}; +use libc::{mode_t, umask, S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR}; -pub fn parse_numeric(fperm: u32, mut mode: &str) -> Result { - let (op, pos) = parse_op(mode, Some('='))?; - mode = mode[pos..].trim().trim_start_matches('0'); - if mode.len() > 4 { - Err(format!("mode is too large ({} > 7777)", mode)) +pub fn parse_numeric(fperm: u32, mut mode: &str, considering_dir: bool) -> Result { + let (op, pos) = parse_op(mode).map_or_else(|_| (None, 0), |(op, pos)| (Some(op), pos)); + mode = mode[pos..].trim(); + let change = if mode.is_empty() { + 0 } else { - match u32::from_str_radix(mode, 8) { - Ok(change) => Ok(match op { - '+' => fperm | change, - '-' => fperm & !change, - '=' => change, - _ => unreachable!(), - }), - Err(err) => Err(err.to_string()), - } + u32::from_str_radix(mode, 8).map_err(|e| e.to_string())? + }; + if change > 0o7777 { + Err(format!("mode is too large ({} > 7777", change)) + } else { + Ok(match op { + Some('+') => fperm | change, + Some('-') => fperm & !change, + // If this is a directory, we keep the setgid and setuid bits, + // unless the mode contains 5 or more octal digits or the mode is "=" + None if considering_dir && mode.len() < 5 => change | (fperm & (0o4000 | 0o2000)), + None | Some('=') => change, + Some(_) => unreachable!(), + }) } } pub fn parse_symbolic( mut fperm: u32, mut mode: &str, + umask: u32, considering_dir: bool, ) -> Result { - #[cfg(unix)] - use libc::umask; - let (mask, pos) = parse_levels(mode); if pos == mode.len() { return Err(format!("invalid mode ({})", mode)); } let respect_umask = pos == 0; - let last_umask = unsafe { umask(0) }; mode = &mode[pos..]; while !mode.is_empty() { - let (op, pos) = parse_op(mode, None)?; + let (op, pos) = parse_op(mode)?; mode = &mode[pos..]; let (mut srwx, pos) = parse_change(mode, fperm, considering_dir); if respect_umask { - srwx &= !(last_umask as u32); + srwx &= !(umask as u32); } mode = &mode[pos..]; match op { '+' => fperm |= srwx & mask, '-' => fperm &= !(srwx & mask), - '=' => fperm = (fperm & !mask) | (srwx & mask), + '=' => { + if considering_dir { + // keep the setgid and setuid bits for directories + srwx |= fperm & (0o4000 | 0o2000); + } + fperm = (fperm & !mask) | (srwx & mask) + } _ => unreachable!(), } } - unsafe { - umask(last_umask); - } Ok(fperm) } @@ -70,9 +75,9 @@ fn parse_levels(mode: &str) -> (u32, usize) { let mut pos = 0; for ch in mode.chars() { mask |= match ch { - 'u' => 0o7700, - 'g' => 0o7070, - 'o' => 0o7007, + 'u' => 0o4700, + 'g' => 0o2070, + 'o' => 0o1007, 'a' => 0o7777, _ => break, }; @@ -84,24 +89,22 @@ fn parse_levels(mode: &str) -> (u32, usize) { (mask, pos) } -fn parse_op(mode: &str, default: Option) -> Result<(char, usize), String> { +fn parse_op(mode: &str) -> Result<(char, usize), String> { let ch = mode .chars() .next() .ok_or_else(|| "unexpected end of mode".to_owned())?; - Ok(match ch { - '+' | '-' | '=' => (ch, 1), - _ => { - let ch = default.ok_or_else(|| { - format!("invalid operator (expected +, -, or =, but found {})", ch) - })?; - (ch, 0) - } - }) + match ch { + '+' | '-' | '=' => Ok((ch, 1)), + _ => Err(format!( + "invalid operator (expected +, -, or =, but found {})", + ch + )), + } } fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) { - let mut srwx = fperm & 0o7000; + let mut srwx = 0; let mut pos = 0; for ch in mode.chars() { match ch { @@ -132,13 +135,19 @@ pub fn parse_mode(mode: &str) -> Result { let fperm = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; let arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; let result = if mode.contains(arr) { - parse_numeric(fperm as u32, mode) + parse_numeric(fperm as u32, mode, true) } else { - parse_symbolic(fperm as u32, mode, true) + parse_symbolic(fperm as u32, mode, get_umask(), true) }; result.map(|mode| mode as mode_t) } +pub fn get_umask() -> u32 { + let mask = unsafe { umask(0) }; + unsafe { umask(mask) }; + mask as u32 +} + #[cfg(test)] mod test { diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 186c645e5..1b8983bc3 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1,5 +1,5 @@ use crate::common::util::*; -use std::fs::{metadata, set_permissions, OpenOptions}; +use std::fs::{metadata, set_permissions, OpenOptions, Permissions}; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; use std::sync::Mutex; @@ -201,11 +201,6 @@ fn test_chmod_ugoa() { before: 0o100000, after: 0o100755, }, - TestCase { - args: vec!["-w", TEST_FILE], - before: 0o100777, - after: 0o100577, - }, TestCase { args: vec!["-x", TEST_FILE], before: 0o100777, @@ -213,6 +208,21 @@ fn test_chmod_ugoa() { }, ]; run_tests(tests); + + // check that we print an error if umask prevents us from removing a permission + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file"); + set_permissions(at.plus("file"), Permissions::from_mode(0o777)).unwrap(); + ucmd.args(&["-w", "file"]) + .fails() + .code_is(1) + // spell-checker:disable-next-line + .stderr_is("chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x"); + assert_eq!( + metadata(at.plus("file")).unwrap().permissions().mode(), + 0o100577 + ); + unsafe { umask(last); } @@ -330,8 +340,8 @@ fn test_chmod_recursive() { .arg("a") .arg("z") .succeeds() - .stderr_contains(&"to 333 (-wx-wx-wx)") - .stderr_contains(&"to 222 (-w--w--w-)"); + .stdout_contains(&"to 0333 (-wx-wx-wx)") + .stdout_contains(&"to 0222 (-w--w--w-)"); assert_eq!(at.metadata("z/y").permissions().mode(), 0o100222); assert_eq!(at.metadata("a/a").permissions().mode(), 0o100222); @@ -350,13 +360,24 @@ fn test_chmod_recursive() { fn test_chmod_non_existing_file() { new_ucmd!() .arg("-R") - .arg("--verbose") .arg("-r,a+w") .arg("does-not-exist") .fails() .stderr_contains(&"cannot access 'does-not-exist': No such file or directory"); } +#[test] +fn test_chmod_non_existing_file_silent() { + new_ucmd!() + .arg("-R") + .arg("--quiet") + .arg("-r,a+w") + .arg("does-not-exist") + .fails() + .no_stderr() + .code_is(1); +} + #[test] fn test_chmod_preserve_root() { new_ucmd!() @@ -490,3 +511,49 @@ fn test_chmod_strip_minus_from_mode() { assert_eq!(test.1, args.join(" ")); } } + +#[test] +fn test_chmod_keep_setgid() { + for &(from, arg, to) in &[ + (0o7777, "777", 0o46777), + (0o7777, "=777", 0o40777), + (0o7777, "0777", 0o46777), + (0o7777, "=0777", 0o40777), + (0o7777, "00777", 0o40777), + (0o2444, "a+wx", 0o42777), + (0o2444, "a=wx", 0o42333), + (0o1444, "g+s", 0o43444), + (0o4444, "u-s", 0o40444), + (0o7444, "a-s", 0o41444), + ] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir"); + set_permissions(at.plus("dir"), Permissions::from_mode(from)).unwrap(); + let r = ucmd.arg(arg).arg("dir").succeeds(); + println!("{}", r.stderr_str()); + assert_eq!(at.metadata("dir").permissions().mode(), to); + } +} + +#[test] +fn test_no_operands() { + new_ucmd!() + .arg("777") + .fails() + .code_is(1) + .stderr_is("chmod: missing operand"); +} + +#[test] +fn test_mode_after_dash_dash() { + let (at, ucmd) = at_and_ucmd!(); + run_single_test( + &TestCase { + args: vec!["--", "-r", TEST_FILE], + before: 0o100777, + after: 0o100333, + }, + at, + ucmd, + ); +}