Merge pull request #2571 from miDeb/chmod/compat

chmod: improve compatibility

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<String>) -> 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),

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<Behavior> {
 
     let specified_mode: Option<u32> = 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
         })?)

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<u32, String> {
+pub fn parse(mode_string: &str, considering_dir: bool, umask: u32) -> Result<u32, String> {
     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)
     }
 }
 

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<u32, String> {
-    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<u32, String> {
+    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<u32, String> {
-    #[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<char>) -> 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<mode_t, String> {
     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 {
 

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,
+    );
+}