diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 874c78dea..65357a576 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -282,7 +282,7 @@ impl Chmoder { // 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(fperm, mode, file.is_dir()) } else { mode::parse_symbolic(fperm, mode, file.is_dir()) }; diff --git a/src/uu/install/src/mode.rs b/src/uu/install/src/mode.rs index b8d5cd839..5c3f8f28f 100644 --- a/src/uu/install/src/mode.rs +++ b/src/uu/install/src/mode.rs @@ -9,7 +9,7 @@ pub fn parse(mode_string: &str, considering_dir: bool) -> Result { // 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) } diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index eb5bfc580..9ddb8540f 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -9,23 +9,26 @@ use libc::{mode_t, 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('='))?; +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(); - match u32::from_str_radix(mode, 8) { - Ok(change) => { - if change > 0o7777 { - Err(format!("mode is too large ({} > 7777", mode)) - } else { - Ok(match op { - '+' => fperm | change, - '-' => fperm & !change, - '=' => change, - _ => unreachable!(), - }) - } - } - Err(err) => Err(err.to_string()), + let change = if mode.is_empty() { + 0 + } else { + 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!(), + }) } } @@ -45,7 +48,7 @@ pub fn parse_symbolic( 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 { @@ -55,7 +58,13 @@ pub fn parse_symbolic( 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!(), } } @@ -70,9 +79,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 +93,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,7 +139,7 @@ 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) }; diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index f0973c712..667f7b476 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; @@ -490,3 +490,26 @@ 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); + } +}