diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index e5c1f997989..f5867b9c3d3 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -319,51 +319,27 @@ impl Chmoder { if let Some(mode) = self.fmode { Ok((mode, mode)) } else { - let cmode_unwrapped = self.cmode.clone().unwrap(); - let mut new_mode = current_mode; - let mut naively_expected_new_mode = current_mode; - - // Where the clause being parsed starts inside `cmode_unwrapped`, so - // that an error can be pointed back at it. - let mut offset = 0; - 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()) { - 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| { - // calculate the new mode as if umask was 0 - let naive_mode = - mode::parse_symbolic(naively_expected_new_mode, mode, 0, is_dir) - .unwrap(); // we know that mode must be valid, so this cannot fail - (m, naive_mode) - }) - }; - - match result { - Ok((mode, naive_mode)) => { - new_mode = mode; - naively_expected_new_mode = naive_mode; + let cmode = self.cmode.clone().unwrap(); + // GNU's mode grammar lives in uucore::mode: a bare octal is the + // whole string, and a clause list must be all-symbolic and + // non-empty. The naive mode is what the clauses would yield with + // umask 0, kept here so the umask diagnostic below can compare. + mode::parse_chmod_with_naive(current_mode, &cmode, is_dir, mode::get_umask()).map_err( + |error| { + if self.quiet { + return ExitCode::new(1); } - Err(error) => { - if self.quiet { - return Err(ExitCode::new(1)); - } - if let Some(args) = &self.args - && let Some((index, operand, offset)) = - self.locate_clause(args, &cmode_unwrapped, clause_start) - && error.render_at(args, index, &operand, offset, &error.to_string()) - { - // The diagnostic is already on stderr; exit quietly. - return Err(ExitCode::new(1)); - } - return Err(USimpleError::new(1, error.to_string())); + if let Some(args) = &self.args + && let Some((index, operand, offset)) = + self.locate_clause(args, &cmode, error.clause_start) + && error.render_at(args, index, &operand, offset, &error.to_string()) + { + // The diagnostic is already on stderr; exit quietly. + return ExitCode::new(1); } - } - } - Ok((new_mode, naively_expected_new_mode)) + USimpleError::new(1, error.to_string()) + }, + ) } } diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 615791c5a0a..9ed82efcd50 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -376,7 +376,9 @@ fn behavior(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult err.to_string()); // When the diagnostic is rendered it is already on stderr; exit quietly. - if !diag_args.is_some_and(|args| err.render_mode_value(args, x, 0, &message)) { + if !diag_args + .is_some_and(|args| err.render_mode_value(args, x, err.clause_start, &message)) + { show_error!("{message}"); } 1 diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 0cb71395ae0..717f7ac3a4b 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -70,7 +70,9 @@ fn get_mode(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult UResult<()> { let message = translate!("mknod-error-invalid-mode", "error" => err.to_string()); if let Some(args) = &diag_args - && err.render_mode_value(args, str_mode, 0, &message) + && err.render_mode_value(args, str_mode, err.clause_start, &message) { // The diagnostic is already on stderr; exit quietly. return ExitCode::new(1); diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 8dfeccb4c94..9cc9a1cf8b8 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -17,14 +17,18 @@ use crate::translate; /// A mode string that does not parse, and the part of it that is at fault. /// -/// `span` is a byte range inside the mode string that was handed to the parser, -/// so that a caller can point a caret at the one clause — often the one -/// character — that broke it. +/// `span` is a byte range inside the clause that broke the parse, so that a +/// caller can point a caret at the offending characters. `clause_start` is +/// where that clause begins inside the whole mode string, which the parser +/// knows but a caller may not; together they let the caret find its spot. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ModeError { pub message: String, pub span: Range, pub kind: ModeErrorKind, + /// Offset of the clause that failed inside the mode string handed to the + /// parser. Zero for a bare mode, which is its own single clause. + pub clause_start: usize, } /// What went wrong, for callers that want to say more than the message does. @@ -36,6 +40,11 @@ pub enum ModeErrorKind { MissingOperator, /// A numeric mode that is not octal, or is out of range. InvalidNumber, + /// A digit-bearing clause inside a symbolic list, where GNU only accepts + /// a bare octal as the whole mode string. + NumericClauseInList, + /// An empty clause in a comma-separated list. + EmptyClause, } impl ModeError { @@ -44,6 +53,7 @@ impl ModeError { message, span, kind, + clause_start: 0, } } @@ -135,8 +145,11 @@ impl ModeError { /// convention in [`crate::diagnostics`]. fn describe(&self) -> (Option, Option) { let label = match self.kind { - // The message already names the expected operators. - ModeErrorKind::InvalidOperator => None, + // The message already says what the clause is and how the grammar + // treats it; a label would only repeat it. + ModeErrorKind::InvalidOperator + | ModeErrorKind::NumericClauseInList + | ModeErrorKind::EmptyClause => None, ModeErrorKind::MissingOperator => Some("mode-diag-label-missing-operator"), ModeErrorKind::InvalidNumber => Some("mode-diag-label-invalid-number"), }; @@ -317,17 +330,25 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) { (srwx, pos) } -/// 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( +/// Apply a mode string to `current_mode`, returning both the result and the +/// mode the same clauses would have produced with umask 0 — what the user +/// asked for before the umask curtailed it, which drives chmod's diagnostic. +fn parse_chmod_inner( current_mode: u32, mode_string: &str, considering_dir: bool, umask: u32, -) -> Result { - let mut new_mode: u32 = current_mode; +) -> Result<(u32, u32), ModeError> { + // A digit makes GNU read the whole string as one bare numeric mode, which + // is only valid without commas; everything else is a list of symbolic + // clauses, and no clause in such a list may be numeric or empty. + if !mode_string.contains(',') && mode_string.chars().any(|c| c.is_ascii_digit()) { + let mode = parse_numeric(current_mode, mode_string, considering_dir)?; + return Ok((mode, mode)); + } - // Split by commas and process each mode part sequentially + let mut new_mode = current_mode; + let mut naive_mode = current_mode; let mut offset = 0; for raw_part in mode_string.split(',') { let start = offset + (raw_part.len() - raw_part.trim_start().len()); @@ -336,18 +357,65 @@ pub fn parse_chmod( let mode_part = raw_part.trim(); if mode_part.is_empty() { - continue; + return Err(ModeError { + clause_start: start, + ..ModeError::new( + format!("invalid mode ({raw_part})"), + 0..0, + ModeErrorKind::EmptyClause, + ) + }); } - - new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) { - parse_numeric(new_mode, mode_part, considering_dir) - } else { - parse_symbolic(new_mode, mode_part, umask, considering_dir) + if mode_part.chars().any(|c| c.is_ascii_digit()) { + return Err(ModeError { + clause_start: start, + ..ModeError::new( + format!("invalid mode ({mode_part})"), + 0..mode_part.len(), + ModeErrorKind::NumericClauseInList, + ) + }); } - .map_err(|err| err.shift(start))?; + + new_mode = parse_symbolic(new_mode, mode_part, umask, considering_dir).map_err(|err| { + ModeError { + clause_start: start, + ..err + } + })?; + // The umask only masks bits, so the same clause parses with umask 0: + // it yields the mode the user asked for, before the umask curtailed it. + naive_mode = parse_symbolic(naive_mode, mode_part, 0, considering_dir) + .expect("the clause parsed above with the caller's umask"); } + Ok((new_mode, naive_mode)) +} + +/// Modify a file mode based on a user-supplied string. +/// +/// GNU accepts either a bare numeric (octal) mode, optionally with a leading +/// `+`, `-` or `=`, or a comma-separated list of symbolic clauses such as +/// "ug+rwX,o+rX". A numeric mode can only be the whole string: a list whose +/// clause contains a digit, or that has an empty clause, is rejected. +pub fn parse_chmod( + current_mode: u32, + mode_string: &str, + considering_dir: bool, + umask: u32, +) -> Result { + parse_chmod_inner(current_mode, mode_string, considering_dir, umask).map(|(mode, _)| mode) +} - Ok(new_mode) +/// Like [`parse_chmod`], but also returns the mode the symbolic clauses would +/// have produced with umask 0, which callers such as chmod need to report a +/// mode curtailed by the umask. +pub fn parse_chmod_with_naive( + current_mode: u32, + mode_string: &str, + considering_dir: bool, + umask: u32, +) -> Result<(u32, u32), ModeError> { + parse_chmod_inner(current_mode, mode_string, considering_dir, umask) } /// Takes a user-supplied string and tries to parse to u32 mode bitmask. @@ -425,8 +493,6 @@ 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); } #[test] @@ -462,18 +528,26 @@ 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); + fn test_parse_rejects_numeric_clause_in_list() { + // GNU only accepts a bare octal as the whole mode; a comma-separated + // list must consist entirely of symbolic clauses. + assert!(parse("644,u+x", false, 0).is_err()); + assert!(parse("u+x,644", false, 0).is_err()); + assert!(parse("a-w,644", false, 0).is_err()); + assert!(parse("644,644", false, 0).is_err()); + assert!(parse("g+s,755", false, 0).is_err()); + assert!(parse("755,g+s", 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_rejects_empty_clauses() { + // GNU rejects an empty mode and any list with an empty clause in it. + assert!(parse("", false, 0).is_err()); + assert!(parse(" ", false, 0).is_err()); + assert!(parse(",,", false, 0).is_err()); + assert!(parse("644,", false, 0).is_err()); + assert!(parse(",644", false, 0).is_err()); + assert!(parse("u+x,,g+x", false, 0).is_err()); } #[test] @@ -512,18 +586,19 @@ mod tests { fn test_parse_complex_combinations() { // 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); + // Symbolic clauses apply in order, so a later one can remove bits + // that an earlier one added. + assert_eq!(parse("a=rw,u-w", false, 0).unwrap(), 0o466); + assert_eq!(parse("a=rw,g-r", false, 0).unwrap(), 0o626); } #[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); + // Test that comma-separated symbolic clauses are applied sequentially + // (a numeric clause in a list is rejected, so relative effects chain). + assert_eq!(parse("u+w,g+r", false, 0).unwrap(), 0o240); - // First add user write, then set to 755 (should override) - assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755); + // A later clause overrides the bits an earlier one set + assert_eq!(parse("u=rw,u+x", false, 0).unwrap(), 0o700); } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index a9ab8403769..0f4abb06db9 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -913,6 +913,28 @@ fn test_gnu_invalid_mode() { scene.ucmd().arg("u+gr").arg("file").fails(); } +#[test] +fn test_gnu_rejects_octal_clause_in_list() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("file"); + for mode in [ + "644,u+x", "u+x,644", "a-w,644", "644,644", "g+s,755", "755,g+s", + ] { + scene.ucmd().arg(mode).arg("file").fails(); + } +} + +#[test] +fn test_gnu_rejects_empty_mode() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("file"); + for mode in ["", " ", ",", "644,", ",644", "u+x,,g+x"] { + scene.ucmd().arg(mode).arg("file").fails(); + } +} + #[test] #[cfg(not(target_os = "android"))] fn test_gnu_options() { @@ -1757,6 +1779,23 @@ mod diagnostics { assert_eq!(result.caret_column(), Some(7), "{stderr}"); } + #[cfg(unix)] + #[test] + fn test_snippet_points_into_a_numeric_clause_in_a_list() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("probe"); + + let result = ucmd + .terminal_sim_stderr() + .args(&["u+x,644", "probe"]) + .fails_with_code(1); + let stderr = result.stderr_str(); + + assert!(stderr.contains("invalid mode"), "{stderr}"); + // The caret lands on the octal clause: `6` is its fifth character. + assert_eq!(result.caret_column(), Some(5), "{stderr}"); + } + #[cfg(unix)] #[test] fn test_snippet_marks_a_clause_with_no_operator() { diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index da638746b56..0c6d6569658 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -304,6 +304,50 @@ fn test_install_mode_comma_separated_directory() { assert_eq!(0o040_775_u32, PermissionsExt::mode(&permissions)); } +#[test] +fn test_install_mode_rejects_octal_clause_in_list() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("source_file"); + at.mkdir("target_dir"); + for mode in [ + "--mode=644,u+x", + "--mode=u+x,644", + "--mode=a-w,644", + "--mode=644,644", + "--mode=g+s,755", + "--mode=755,g+s", + ] { + scene + .ucmd() + .args(&["source_file", "target_dir"]) + .arg(mode) + .fails(); + } +} + +#[test] +fn test_install_mode_rejects_empty_mode() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("source_file"); + at.mkdir("target_dir"); + for mode in [ + "--mode=", + "--mode= ", + "--mode=,,", + "--mode=644,", + "--mode=,644", + "--mode=u+x,,g+x", + ] { + scene + .ucmd() + .args(&["source_file", "target_dir"]) + .arg(mode) + .fails(); + } +} + #[test] fn test_install_mode_symbolic_ignore_umask() { let (at, mut ucmd) = at_and_ucmd!(); diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index c0601a850d8..90e7db73338 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -1083,6 +1083,24 @@ fn test_mkdir_inside_inexistent_dir() { } // The mode is only parsed where a mode means something. +#[cfg(unix)] +#[test] +fn test_mkdir_rejects_octal_clause_in_list() { + for mode in [ + "644,u+x", "u+x,644", "a-w,644", "644,644", "g+s,755", "755,g+s", + ] { + new_ucmd!().args(&["-m", mode, "some_dir"]).fails(); + } +} + +#[cfg(unix)] +#[test] +fn test_mkdir_rejects_empty_mode() { + for mode in ["", " ", ",", "644,", ",644", "u+x,,g+x"] { + new_ucmd!().args(&["-m", mode, "some_dir"]).fails(); + } +} + #[cfg(unix)] #[cfg(all(feature = "feat_diagnostics", not(wasi_runner)))] mod diagnostics { diff --git a/tests/by-util/test_mkfifo.rs b/tests/by-util/test_mkfifo.rs index 13d43f5a59a..3cb91a1f855 100644 --- a/tests/by-util/test_mkfifo.rs +++ b/tests/by-util/test_mkfifo.rs @@ -43,6 +43,21 @@ fn test_create_one_fifo_with_invalid_mode() { .stderr_contains("invalid mode"); } +#[test] +fn test_create_one_fifo_rejects_octal_clause_in_list_and_empty_modes() { + for mode in [ + "644,u+x", "u+x,644", "a-w,644", "644,644", "g+s,755", "755,g+s", "u+x,,g+x", "644,", + ",644", + ] { + new_ucmd!() + .arg("abcd") + .arg("-m") + .arg(mode) + .fails() + .stderr_contains("invalid mode"); + } +} + #[test] fn test_create_one_fifo_with_non_file_permission_mode() { new_ucmd!() diff --git a/tests/by-util/test_mknod.rs b/tests/by-util/test_mknod.rs index 49fdbc243f9..3ac2ec4f204 100644 --- a/tests/by-util/test_mknod.rs +++ b/tests/by-util/test_mknod.rs @@ -138,6 +138,23 @@ fn test_mknod_invalid_mode() { .stderr_contains("invalid mode"); } +#[test] +fn test_mknod_rejects_octal_clause_in_list_and_empty_modes() { + for mode in [ + "644,u+x", "u+x,644", "a-w,644", "644,644", "g+s,755", "755,g+s", "u+x,,g+x", "644,", + ",644", + ] { + new_ucmd!() + .arg("--mode") + .arg(mode) + .arg("test_file") + .arg("p") + .fails() + .code_is(1) + .stderr_contains("invalid mode"); + } +} + #[test] fn test_mknod_mode_permissions() { for test_mode in [0o0666, 0o0000, 0o0444, 0o0004, 0o0040, 0o0400, 0o0644] {