diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index c98e4af89e5..4e6b62cb7fb 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -326,6 +326,16 @@ impl Chmoder { let mut new_mode = current_mode; let mut naively_expected_new_mode = current_mode; + if let Err(error) = mode::check_clauses(&cmode_unwrapped) { + // Report it like a clause error, from where the fault starts + let clause_start = error.span.start; + let error = mode::ModeError { + span: 0..error.span.len(), + ..error + }; + return Err(self.mode_error(&cmode_unwrapped, clause_start, &error)); + } + // Where the clause being parsed starts inside `cmode_unwrapped`, so // that an error can be pointed back at it. let mut offset = 0; @@ -351,18 +361,7 @@ impl Chmoder { naively_expected_new_mode = naive_mode; } 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())); + return Err(self.mode_error(&cmode_unwrapped, clause_start, &error)); } } } @@ -370,6 +369,27 @@ impl Chmoder { } } + /// The error to return for `error`, found in the clause that starts at + /// `clause_start` inside `cmode`. Its span is relative to that clause. + fn mode_error( + &self, + cmode: &str, + clause_start: usize, + error: &mode::ModeError, + ) -> Box { + if self.quiet { + return ExitCode::new(1); + } + if let Some(args) = &self.args + && let Some((index, operand, offset)) = self.locate_clause(args, cmode, clause_start) + && error.render_at(args, index, &operand, offset, &error.to_string()) + { + // The diagnostic is already on stderr; exit quietly. + return ExitCode::new(1); + } + USimpleError::new(1, error.to_string()) + } + /// Map `clause_start` — an offset into `cmode`, the joined mode string — /// back to the argument the failing clause came from. /// diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index b6ce07aa9d5..498ec32c6cf 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -38,6 +38,10 @@ pub enum ModeErrorKind { MissingOperator, /// A numeric mode that is not octal, or is out of range. InvalidNumber, + /// Nothing between two commas, or at either end of the mode. + EmptyClause, + /// An octal mode without an operator, next to other clauses. + OctalInList, } impl ModeError { @@ -177,8 +181,10 @@ 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 is wrong. + ModeErrorKind::InvalidOperator + | ModeErrorKind::EmptyClause + | ModeErrorKind::OctalInList => None, ModeErrorKind::MissingOperator => Some("mode-diag-label-missing-operator"), ModeErrorKind::InvalidNumber => Some("mode-diag-label-invalid-number"), }; @@ -359,6 +365,44 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) { (srwx, pos) } +/// The comma-separated clauses of `mode`, trimmed, each with where it starts +/// inside `mode`. +fn clauses(mode: &str) -> impl Iterator { + let mut offset = 0; + mode.split(',').map(move |raw| { + let start = offset + (raw.len() - raw.trim_start().len()); + // Past the clause and the comma that followed it. + offset += raw.len() + 1; + (start, raw.trim()) + }) +} + +/// Check how the comma-separated clauses of `mode` fit together: `+644,u+x` +/// is valid, `u+x,` and `644,u+x` are not. The error's span is inside `mode`. +/// +/// Empty clauses are looked for first, so `644,` points at the missing +/// clause rather than at `644`. A blank clause counts as empty. +pub fn check_clauses(mode: &str) -> Result<(), ModeError> { + if let Some((start, _)) = clauses(mode).find(|(_, clause)| clause.is_empty()) { + return Err(ModeError::new( + format!("invalid mode ({mode})"), + start..start, + ModeErrorKind::EmptyClause, + )); + } + if mode.contains(',') + && let Some((start, clause)) = + clauses(mode).find(|(_, clause)| clause.starts_with(|c: char| c.is_ascii_digit())) + { + return Err(ModeError::new( + format!("invalid mode ({mode})"), + start..start + clause.len(), + ModeErrorKind::OctalInList, + )); + } + 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( @@ -367,20 +411,11 @@ pub fn parse_chmod( considering_dir: bool, umask: u32, ) -> Result { + check_clauses(mode_string)?; let mut new_mode: u32 = current_mode; - // Split by commas and process each mode part sequentially - let mut offset = 0; - 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; - } - + // Process each mode part sequentially + for (start, mode_part) in clauses(mode_string) { new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) { parse_numeric(new_mode, mode_part, considering_dir) } else { @@ -494,6 +529,7 @@ pub fn with_umask(mask: u32, operation: impl FnOnce() -> T) -> T { #[cfg(test)] mod tests { + use super::ModeErrorKind; use super::parse; use super::parse_chmod; @@ -530,7 +566,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] @@ -567,17 +603,51 @@ 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); + // An octal mode next to other clauses 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("644,u+x", false, 0).unwrap_err().kind, + ModeErrorKind::OctalInList + ); + assert_eq!( + parse("u+rw,755", false, 0).unwrap_err().kind, + ModeErrorKind::OctalInList + ); + assert_eq!( + parse("+644,644", false, 0).unwrap_err().kind, + ModeErrorKind::OctalInList + ); + } + + #[test] + fn test_parse_empty_clauses() { + for mode in [ + "", + " ", + ",", + ",,", + "u+x,", + ",u+x", + "u+x,,g+x", + "u+x, ,g+x", + ",644", + "644,", + "644,,u+x", + ] { + let err = parse(mode, false, 0).unwrap_err(); + assert_eq!(err.kind, ModeErrorKind::EmptyClause, "{mode:?}"); + } + // The caret goes under the missing clause, not under the octal + assert_eq!(parse("644,", false, 0).unwrap_err().span, 4..4); } #[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_error_spans() { + // The span is inside the whole mode, not the clause + assert_eq!(parse("u+x,,g+x", false, 0).unwrap_err().span, 4..4); + assert_eq!(parse("u+x,644", false, 0).unwrap_err().span, 4..7); + assert_eq!(parse("u+x, ,g+x", false, 0).unwrap_err().span, 5..5); } #[test] @@ -617,18 +687,18 @@ 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); } /// Reads the umask without going through the helpers under test. diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index afbf7e23e1e..eb8c15f819f 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -984,6 +984,59 @@ fn test_chmod_invalid_mode_checked_before_any_file() { .stderr_only("chmod: invalid operator (expected +, -, or =, but found ?)\n"); } +#[test] +fn test_mode_list_with_empty_clause_or_bare_octal() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("file"); + set_permissions(at.plus("file"), Permissions::from_mode(0o640)).unwrap(); + + for mode in [ + "", ",", "u+x,", ",u+x", "u+x,,g+w", "u+x, ", "644,", "644,u+x", "u+x,644", "+644,644", + " 644,u+x", + ] { + scene + .ucmd() + .arg(mode) + .arg("file") + .fails_with_code(1) + .no_stdout() + .stderr_is(format!("chmod: invalid mode ({mode})\n")); + assert_eq!( + at.metadata("file").permissions().mode(), + 0o100_640, + "{mode:?}" + ); + } + + scene.ucmd().arg("=644,u+x").arg("file").succeeds(); + assert_eq!(at.metadata("file").permissions().mode(), 0o100_744); +} + +#[test] +fn test_mode_list_rejected_before_files() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.symlink_file("nowhere", "dangling"); + + for mode in [",", "u+x,", "644,u+x"] { + scene + .ucmd() + .arg(mode) + .arg("missing") + .fails_with_code(1) + .stderr_is(format!("chmod: invalid mode ({mode})\n")); + } + + scene + .ucmd() + .arg("-h") + .arg("u+x,") + .arg("dangling") + .fails_with_code(1) + .stderr_is("chmod: invalid mode (u+x,)\n"); +} + #[test] #[cfg(not(target_os = "android"))] fn test_gnu_options() { diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index a3b9c5657d4..932b9daa857 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -304,6 +304,38 @@ fn test_install_mode_comma_separated_directory() { assert_eq!(0o040_775_u32, PermissionsExt::mode(&permissions)); } +#[test] +fn test_install_mode_list_with_empty_clause_or_bare_octal() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + for mode in [ + "", ",", "u+x,", ",u+x", "u+x,,g+w", "644,u+x", "u+x,644", "+644,644", " 644,u+x", + ] { + scene + .ucmd() + .arg("-d") + .arg("-m") + .arg(mode) + .arg("test_dir") + .fails_with_code(1) + .stderr_is(format!( + "install: Invalid mode string: invalid mode ({mode})\n" + )); + assert!(!at.dir_exists("test_dir"), "{mode:?}"); + } + + scene + .ucmd() + .arg("-d") + .arg("-m") + .arg("=644,u+x") + .arg("test_dir") + .succeeds(); + let permissions = at.metadata("test_dir").permissions(); + assert_eq!(0o040_744_u32, PermissionsExt::mode(&permissions)); +} + #[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 4d2722a858a..0701c64250d 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -260,6 +260,35 @@ fn test_symbolic_mode() { assert_eq!(perms, 0o40777); } +#[test] +#[cfg(not(windows))] +#[cfg_attr(wasi_runner, ignore = "WASI: st_mode has no real permission bits")] +fn test_mode_list_with_empty_clause_or_bare_octal() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + for mode in [ + "", ",", "u+x,", ",u+x", "u+x,,g+w", "644,u+x", "u+x,644", " 644,u+x", + ] { + scene + .ucmd() + .arg("-m") + .arg(mode) + .arg("test_dir") + .fails_with_code(1) + .stderr_is(format!("mkdir: invalid mode '{mode}'\n")); + assert!(!at.dir_exists("test_dir"), "{mode:?}"); + } + + scene + .ucmd() + .arg("-m") + .arg("=644,u+x") + .arg("test_dir") + .succeeds(); + assert_eq!(at.metadata("test_dir").permissions().mode(), 0o40744); +} + #[test] #[cfg(not(windows))] #[cfg_attr(wasi_runner, ignore = "WASI: st_mode has no real permission bits")] diff --git a/tests/by-util/test_mkfifo.rs b/tests/by-util/test_mkfifo.rs index 58b93d358f1..215b3a74c05 100644 --- a/tests/by-util/test_mkfifo.rs +++ b/tests/by-util/test_mkfifo.rs @@ -43,6 +43,32 @@ fn test_create_one_fifo_with_invalid_mode() { .stderr_contains("invalid mode"); } +#[test] +fn test_create_fifo_with_empty_clause_or_bare_octal() { + let scene = TestScenario::new(util_name!()); + + for mode in [ + "", ",", "u+x,", ",u+x", "u+x,,g+w", "644,u+x", "u+x,644", "+644,644", " 644,u+x", + ] { + scene + .ucmd() + .arg("-m") + .arg(mode) + .arg("abcd") + .fails_with_code(1) + .stderr_is(format!("mkfifo: invalid mode: invalid mode ({mode})\n")); + assert!(!scene.fixtures.file_exists("abcd"), "{mode:?}"); + } + + scene + .ucmd() + .arg("-m") + .arg("=644,u+x") + .arg("abcd") + .succeeds(); + assert!(scene.fixtures.is_fifo("abcd")); +} + #[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 9c9bc9730a3..6515a14ba41 100644 --- a/tests/by-util/test_mknod.rs +++ b/tests/by-util/test_mknod.rs @@ -257,6 +257,35 @@ fn test_mknod_fifo_with_umask() { test_fifo_creation(0o777, "p---------"); // spell-checker:disable-line } +#[test] +fn test_mknod_mode_list_with_empty_clause_or_bare_octal() { + let ts = TestScenario::new(util_name!()); + + for mode in [ + "", ",", "u+x,", ",u+x", "u+x,,g+w", "644,u+x", "u+x,644", "+644,644", " 644,u+x", + ] { + ts.ucmd() + .arg("-m") + .arg(mode) + .arg("test_file") + .arg("p") + .fails_with_code(1) + .stderr_is(format!("mknod: invalid mode (invalid mode ({mode}))\n")); + assert!(!ts.fixtures.file_exists("test_file"), "{mode:?}"); + } + + ts.ucmd() + .arg("-m") + .arg("=644,u+x") + .arg("test_file") + .arg("p") + .succeeds(); + assert_eq!( + ts.fixtures.metadata("test_file").permissions().mode() & 0o777, + 0o744 + ); +} + #[test] #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] fn test_mknod_selinux() {