diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index f586ef8a681..9a733d499c4 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -325,11 +325,15 @@ impl Chmoder { // Where the clause being parsed starts inside `cmode_unwrapped`, so // that an error can be pointed back at it. let mut offset = 0; + let clauses = cmode_unwrapped.split(',').count(); for mode in cmode_unwrapped.split(',') { let clause_start = offset; offset += mode.len() + 1; // past the clause and its comma - let result = if mode.chars().any(|c| c.is_ascii_digit()) { + let result = if let Err(error) = mode::check_clause(&cmode_unwrapped, mode, clauses) + { + Err(error) + } else if mode.chars().any(|c| c.is_ascii_digit()) { mode::parse_numeric(new_mode, mode, is_dir).map(|v| (v, v)) } else { mode::parse_symbolic(new_mode, mode, mode::get_umask(), is_dir).map(|m| { diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 05867346331..31f7f9e9330 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -359,6 +359,29 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) { (srwx, pos) } +/// Check one comma-separated `clause` of `mode`, which has `clauses` clauses. +/// +/// A clause cannot be empty (`u+x,`, `,u+x`, `u+x,,g+w`), and a numeric mode +/// without an operator (`644`) must be the whole mode, so `644,u+x` and `1,2` +/// are invalid. The error's span is relative to the clause. +pub fn check_clause(mode: &str, clause: &str, clauses: usize) -> Result<(), ModeError> { + if clause.is_empty() { + return Err(ModeError::new( + format!("invalid mode ({mode})"), + 0..0, + ModeErrorKind::MissingOperator, + )); + } + if clauses > 1 && clause.starts_with(|c: char| c.is_ascii_digit()) { + return Err(ModeError::new( + format!("invalid mode ({mode})"), + 0..clause.len(), + ModeErrorKind::InvalidNumber, + )); + } + Ok(()) +} + /// Modify a file mode based on a user-supplied string. /// Supports comma-separated mode strings like "ug+rwX,o+rX" (same as chmod). pub fn parse_chmod( @@ -371,15 +394,14 @@ pub fn parse_chmod( // Split by commas and process each mode part sequentially let mut offset = 0; + let clauses = mode_string.split(',').count(); for raw_part in mode_string.split(',') { let start = offset + (raw_part.len() - raw_part.trim_start().len()); // Past the part and the comma that followed it. offset += raw_part.len() + 1; let mode_part = raw_part.trim(); - if mode_part.is_empty() { - continue; - } + check_clause(mode_string, mode_part, clauses).map_err(|err| err.shift(start))?; new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) { parse_numeric(new_mode, mode_part, considering_dir) @@ -468,7 +490,7 @@ mod tests { // Numeric mode with - operator (starting from 0, so nothing to remove) assert_eq!(parse("-4", false, 0).unwrap(), 0); // But if we first set a mode, then remove bits - assert_eq!(parse("644,-4", false, 0).unwrap(), 0o640); + assert_eq!(parse("=644,-4", false, 0).unwrap(), 0o640); } #[test] @@ -505,17 +527,31 @@ mod tests { #[test] fn test_parse_mixed_numeric_and_symbolic() { - // Mix of numeric and symbolic modes - assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744); - assert_eq!(parse("u+rw,755", false, 0).unwrap(), 0o755); + // Mix of numeric and symbolic modes: in a list, a numeric mode needs + // an operator + assert_eq!(parse("=644,u+x", false, 0).unwrap(), 0o744); + assert_eq!(parse("u+rw,=755", false, 0).unwrap(), 0o755); + assert_eq!(parse("u+x,+644", false, 0).unwrap(), 0o744); + + // A numeric mode without an operator must be the whole mode, as in GNU + assert!(parse("644,u+x", false, 0).is_err()); + assert!(parse("u+rw,755", false, 0).is_err()); + assert!(parse("1,2", false, 0).is_err()); } #[test] - fn test_parse_empty_string() { - // Empty string should return 0 - assert_eq!(parse("", false, 0).unwrap(), 0); - assert_eq!(parse(" ", false, 0).unwrap(), 0); - assert_eq!(parse(",,", false, 0).unwrap(), 0); + fn test_parse_empty_clauses() { + // An empty mode or an empty clause is invalid, as in GNU + assert!(parse("", false, 0).is_err()); + assert!(parse(" ", false, 0).is_err()); + assert!(parse(",,", false, 0).is_err()); + assert!(parse("u+x,", false, 0).is_err()); + assert!(parse(",u+x", false, 0).is_err()); + assert!(parse("u+r,,g+w", false, 0).is_err()); + + // `=` and a lone operator are clauses, not empty ones + assert_eq!(parse("=,u+x", false, 0).unwrap(), 0o100); + assert_eq!(parse("+,-", false, 0).unwrap(), 0); } #[test] @@ -555,17 +591,17 @@ mod tests { // Complex real-world examples assert_eq!(parse("u=rwx,g=rx,o=r", false, 0).unwrap(), 0o754); // To test removal, we need to first set permissions, then remove them - assert_eq!(parse("644,a-w", false, 0).unwrap(), 0o444); - assert_eq!(parse("644,g-r", false, 0).unwrap(), 0o604); + assert_eq!(parse("=644,a-w", false, 0).unwrap(), 0o444); + assert_eq!(parse("=644,g-r", false, 0).unwrap(), 0o604); } #[test] fn test_parse_sequential_application() { // Test that comma-separated modes are applied sequentially // First set to 644, then add execute for user - assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744); + assert_eq!(parse("=644,u+x", false, 0).unwrap(), 0o744); // First add user write, then set to 755 (should override) - assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755); + assert_eq!(parse("u+w,=755", false, 0).unwrap(), 0o755); } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index f4e07d9bdbf..9ca00551f43 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1865,3 +1865,27 @@ mod diagnostics { .no_output(); } } + +#[test] +fn test_chmod_rejects_empty_and_bare_numeric_clauses() { + // Rejected by GNU too: an empty clause, and a numeric mode without an + // operator in a list of clauses + for mode in ["u+x,", ",u+x", "u+r,,g+w", ",", "644,u+x", "u+x,644", "1,2"] { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file"); + set_permissions(at.plus("file"), Permissions::from_mode(0o644)).unwrap(); + ucmd.args(&[mode, "file"]) + .fails_with_code(1) + .stderr_only(format!("chmod: invalid mode ({mode})\n")); + assert_eq!(at.metadata("file").mode() & 0o7777, 0o644, "{mode}"); + } +} + +#[test] +fn test_chmod_numeric_mode_with_operator_in_a_list() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file"); + set_permissions(at.plus("file"), Permissions::from_mode(0o600)).unwrap(); + ucmd.args(&["u+x,=644,g+w", "file"]).succeeds().no_output(); + assert_eq!(at.metadata("file").mode() & 0o7777, 0o664); +} diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index b12e90c280e..0463ec82db3 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -1263,3 +1263,16 @@ fn test_mkdir_concurrent_non_recursive() { ); } } + +#[test] +fn test_mkdir_mode_rejects_empty_and_bare_numeric_clauses() { + // Rejected by GNU too: an empty clause, and a numeric mode without an + // operator in a list of clauses + for mode in ["u+x,", ",u+x", "u+r,,g+w", ",", "644,u+x", "u+x,644", "1,2"] { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-m", mode, "dir"]) + .fails_with_code(1) + .stderr_only(format!("mkdir: invalid mode '{mode}'\n")); + assert!(!at.dir_exists("dir"), "{mode}"); + } +}