chmod: show an error if a permission wasn't removed due to umask

src/uu/chmod/src/chmod.rs

@@ -255,7 +255,9 @@ impl Chmoder {
     }
     #[cfg(any(unix, target_os = "redox"))]
     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 +280,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, file.is_dir())
+                        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 +313,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, false),
+                        display_permissions_unix(naively_expected_new_mode, false)
+                    );
+                    return Err(1);
+                }
             }
         }
 

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, USimpleError};
+use uucore::mode::get_umask;
 use uucore::perms::{wrap_chown, Verbosity, VerbosityLevel};
 
 use libc::{getegid, geteuid};
@@ -378,7 +379,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, 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

@@ -7,7 +7,7 @@
 
 // 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, considering_dir: bool) -> Result<u32, String> {
     let (op, pos) = parse_op(mode).map_or_else(|_| (None, 0), |(op, pos)| (Some(op), pos));
@@ -35,24 +35,21 @@ pub fn parse_numeric(fperm: u32, mut mode: &str, considering_dir: bool) -> Resul
 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)?;
         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 {
@@ -68,9 +65,6 @@ pub fn parse_symbolic(
             _ => unreachable!(),
         }
     }
-    unsafe {
-        umask(last_umask);
-    }
     Ok(fperm)
 }
 
@@ -141,11 +135,17 @@ pub fn parse_mode(mode: &str) -> Result<mode_t, String> {
     let result = if mode.contains(arr) {
         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
+}
+
 #[cfg(test)]
 mod test {
 

tests/by-util/test_chmod.rs

@@ -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);
     }