From a34375c0579d3a3fc3c3df3fb25b9dd7c556d39d Mon Sep 17 00:00:00 2001 From: dhruv-15-03 Date: Mon, 3 Aug 2026 22:06:11 +0530 Subject: [PATCH] chmod: only report umask-curtailed modes for option-like operands GNU prints "new permissions are X, not Y" and exits 1 when a umask-relative mode such as `-w` could not remove everything it asked for, but it only does so when the mode was written in the option-like form, e.g. `chmod -w file`. When the mode arrives as an ordinary positional operand the message is not printed and the exit status is 0. Comparing 17 mode spellings against GNU coreutils 8.32 with a file at 0666 and umask 022, 8 diverged, all in the same direction: uutils reported and exited 1 where GNU was silent and exited 0. That includes `chmod u+x,-w file`, an entirely ordinary command with no `--` involved. `extract_negative_modes` already returns `Some(..)` exactly when it lifted option-like mode fragments out of the argument list, which is the predicate needed, so record it and gate the diagnostic on it. Also add the complete 26-row operand matrix from the issue report as a regression test. Those rows already pass, but nothing pinned them. Refs #3147 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/uu/chmod/src/chmod.rs | 18 ++++- tests/by-util/test_chmod.rs | 135 ++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 2 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 7f6dc3ae80..95e88785b6 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -136,6 +136,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }; let modes = matches.get_one::(options::MODE); + // Whether the mode reached us as an option-like operand ("chmod -w f") rather than as an + // ordinary positional operand ("chmod -- -w f"). This decides whether the umask diagnostic + // below is emitted; see the comment on `option_like_mode`. + let option_like_mode = parsed_cmode.is_some(); let cmode = if let Some(parsed_cmode) = parsed_cmode { parsed_cmode } else { @@ -173,6 +177,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { recursive, fmode, cmode, + option_like_mode, traverse_symlinks, dereference, }; @@ -270,6 +275,11 @@ struct Chmoder { recursive: bool, fmode: Option, cmode: Option, + /// Set when the mode was given as an option-like operand, e.g. `chmod -w f`, instead of as a + /// plain positional operand, e.g. `chmod -- -w f`. GNU only reports a mode whose effect was + /// curtailed by the umask for the first spelling: the second one is unambiguous, so there is + /// nothing to warn about. + option_like_mode: bool, traverse_symlinks: TraverseSymlinks, dereference: bool, } @@ -787,8 +797,12 @@ impl Chmoder { } else { 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 { + // A bare mode such as `-w` is umask-relative, so the umask can keep permissions that + // the user asked to drop. GNU reports that as an error, but only when the mode was + // written in the option-like form (`chmod -w f`), where it doubles as a hint that the + // argument was consumed as a mode. After `--` the operand is unambiguous and GNU stays + // silent, so the diagnostic is suppressed here too. + if self.option_like_mode && (new_mode & !naively_expected_new_mode) != 0 { return Err(ChmodError::NewPermissions( file.into(), display_permissions_unix(new_mode, false), diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 6f5ab57e05..21424e020a 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -887,6 +887,141 @@ fn test_gnu_special_options() { scene.ucmd().arg("--").arg("--").fails(); } +/// Every row of the operand matrix from +/// . +/// +/// The left column is the argument list, the right column is the exact list of operands, in order, +/// that must end up being changed. Three files named `f`, `--` and `-w` exist in every case, so a +/// row that expects `[]` is asserting that the arguments are rejected outright rather than being +/// resolved to some file that happens to exist. None of the rows are redundant: they only differ +/// pairwise in where a `--` sits, which is precisely what decides whether a leading-hyphen argument +/// is a mode or a file name. +#[test] +#[cfg(not(target_os = "android"))] +fn test_gnu_usage_matrix() { + let matrix: &[(&[&str], &[&str])] = &[ + (&["--"], &[]), + (&["--", "--"], &[]), + (&["--", "--", "--", "f"], &["--", "f"]), + (&["--", "--", "-w", "f"], &["-w", "f"]), + (&["--", "--", "f"], &["f"]), + (&["--", "-w"], &[]), + (&["--", "-w", "--", "f"], &["--", "f"]), + (&["--", "-w", "-w", "f"], &["-w", "f"]), + (&["--", "-w", "f"], &["f"]), + (&["--", "f"], &[]), + (&["-w"], &[]), + (&["-w", "--"], &[]), + (&["-w", "--", "--", "f"], &["--", "f"]), + (&["-w", "--", "-w", "f"], &["-w", "f"]), + (&["-w", "--", "f"], &["f"]), + (&["-w", "-w"], &[]), + (&["-w", "-w", "--", "f"], &["f"]), + (&["-w", "-w", "-w", "f"], &["f"]), + (&["-w", "-w", "f"], &["f"]), + (&["-w", "f"], &["f"]), + (&["f"], &[]), + (&["f", "--"], &[]), + (&["f", "-w"], &["f"]), + (&["f", "f"], &[]), + (&["u+gr", "f"], &[]), + (&["ug,+x", "f"], &[]), + ]; + + for (args, expected) in matrix { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + // 0o644 has no group or other write bit, so `-w` lands on 0o444 under any umask. This test + // is about which operands get picked, not about umask arithmetic. + for name in ["f", "--", "-w"] { + make_file(&at.plus_as_string(name), 0o644); + } + + let result = scene.ucmd().arg("-v").args(args).run(); + let context = format!("chmod -v {}", args.join(" ")); + + // `-v` names every operand it visits, whether or not the mode ends up changing anything. + let visited: Vec<&str> = result + .stdout_str() + .lines() + .filter_map(|line| line.strip_prefix("mode of '")) + .filter_map(|rest| rest.split('\'').next()) + .collect(); + assert_eq!(visited, *expected, "{context}: acted on the wrong operands"); + + // A row that names no operand is an error (a missing or invalid mode), never a silent + // no-op. + assert_eq!( + result.succeeded(), + !expected.is_empty(), + "{context}: unexpected exit status" + ); + + // Independently of `-v`, nothing outside the expected list may be touched. This is what + // catches an implementation that mistakes the file named `--` for a separator. + for name in ["f", "--", "-w"] { + if !expected.contains(&name) { + assert_eq!( + at.metadata(name).permissions().mode() & 0o7777, + 0o644, + "{context}: {name} should have been left alone" + ); + } + } + } +} + +/// `chmod` warns that the umask kept bits the mode asked to remove only when the mode was written +/// in the option-like form, i.e. as a leading-hyphen argument before any `--`. Once `--` has been +/// seen the operand is unambiguously a mode, and the change is applied silently. +#[test] +#[cfg(not(target_os = "android"))] +fn test_umask_conflict_reported_only_for_option_like_mode() { + // (arguments, resulting permission bits, whether the umask conflict is reported) + let cases: &[(&[&str], u32, bool)] = &[ + (&["-w", "file"], 0o466, true), + // A `--` after the mode does not retroactively make it an ordinary operand. + (&["-w", "--", "file"], 0o466, true), + (&["file", "-w"], 0o466, true), + (&["-w", "-w", "--", "file"], 0o466, true), + (&["--", "-w", "file"], 0o466, false), + (&["--", "-rw", "file"], 0o022, false), + // What matters is whether the argument itself began with a hyphen, not whether the mode + // contains an umask-relative clause: these two modes do the same thing, and only the one + // that looks like an option is reported. + (&["u+x,-w", "file"], 0o566, false), + (&["-w,u+x", "file"], 0o566, true), + (&["--", "-w,u+x", "file"], 0o566, false), + ]; + + for (args, expected_mode, reported) in cases { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + // 0o666 is required: on 0o644 the umask has nothing left to keep, so the conflict never + // arises and every one of these cases would pass vacuously. + make_file(&at.plus_as_string("file"), 0o666); + + let result = scene.ucmd().umask(0o022).args(args).run(); + let context = format!("chmod {}", args.join(" ")); + + assert_eq!( + at.metadata("file").permissions().mode() & 0o7777, + *expected_mode, + "{context}: wrong resulting permissions" + ); + if *reported { + result.code_is(1); + assert!( + result.stderr_str().contains("new permissions are"), + "{context}: expected the umask conflict to be reported, got {:?}", + result.stderr_str() + ); + } else { + result.success().no_stderr(); + } + } +} + #[test] fn test_chmod_dereference_symlink() { let scene = TestScenario::new(util_name!());