diff --git a/crates/cli/src/frontend/execution/drive/metadata/compute.rs b/crates/cli/src/frontend/execution/drive/metadata/compute.rs index 4eb654e01..8a94bcad4 100644 --- a/crates/cli/src/frontend/execution/drive/metadata/compute.rs +++ b/crates/cli/src/frontend/execution/drive/metadata/compute.rs @@ -157,9 +157,11 @@ fn parse_chmod_specs( modifiers = Some(parsed); } } - Err(error) => { - let formatted = - format!("failed to parse --chmod specification '{spec_text}': {error}"); + Err(_error) => { + // upstream: options.c:1762-1766 - a rejected spec yields + // `Invalid argument passed to --chmod (%s)` (raw arg) then + // exit_cleanup(RERR_SYNTAX). + let formatted = format!("Invalid argument passed to --chmod ({spec_text})"); return Err(core::rsync_error!(1, formatted).with_role(core::message::Role::Client)); } } diff --git a/crates/cli/src/frontend/tests/run.rs b/crates/cli/src/frontend/tests/run.rs index 020409898..f4448db23 100644 --- a/crates/cli/src/frontend/tests/run.rs +++ b/crates/cli/src/frontend/tests/run.rs @@ -1,6 +1,9 @@ use super::common::*; use super::*; +// upstream: options.c:1762-1766 - a rejected --chmod arg prints +// `Invalid argument passed to --chmod (%s)` (verbatim raw arg) and exits +// RERR_SYNTAX (1). We assert the message and exit code byte-for-byte. #[test] fn run_reports_invalid_chmod_specification() { use tempfile::tempdir; @@ -10,17 +13,25 @@ fn run_reports_invalid_chmod_specification() { let destination = tmp.path().join("dest.txt"); std::fs::write(&source, b"data").expect("write source"); - let (code, stdout, stderr) = run_with_args([ - OsString::from(RSYNC), - OsString::from("--chmod=a+q"), - source.into_os_string(), - destination.into_os_string(), - ]); - - assert_eq!(code, 1); - assert!(stdout.is_empty()); - let rendered = String::from_utf8(stderr).expect("diagnostic utf8"); - assert!(rendered.contains("failed to parse --chmod specification")); + // A category letter (u/g/o) in the permission half is rejected exactly as + // upstream chmod.c STATE_2ND_HALF does; rsync has no chmod(1)-style copy + // form. Also exercise a plainly bogus letter. + for spec in ["a+q", "g=u", "o=g", "u+g"] { + let (code, stdout, stderr) = run_with_args([ + OsString::from(RSYNC), + OsString::from(format!("--chmod={spec}")), + source.clone().into_os_string(), + destination.clone().into_os_string(), + ]); + + assert_eq!(code, 1, "`--chmod={spec}` must exit RERR_SYNTAX"); + assert!(stdout.is_empty()); + let rendered = String::from_utf8(stderr).expect("diagnostic utf8"); + assert!( + rendered.contains(&format!("Invalid argument passed to --chmod ({spec})")), + "diagnostic for `{spec}` was: {rendered}" + ); + } } #[test] diff --git a/crates/metadata/src/chmod/apply.rs b/crates/metadata/src/chmod/apply.rs index dcdfd629c..dc8d2e98c 100644 --- a/crates/metadata/src/chmod/apply.rs +++ b/crates/metadata/src/chmod/apply.rs @@ -23,27 +23,6 @@ pub(crate) fn apply_clauses(_clauses: &[Clause], mode: u32, _file_type: std::fs: mode } -/// Resolves the copy-from-category bits for a clause against the live mode. -/// -/// upstream: chmod.c `mode_copy_bits()` - extracts the three rwx bits of each -/// selected source class, then distributes them across the destination -/// classes, finally masking with the clause's copy AND (`CHMOD_BITS` or -/// `~umask`). -#[cfg(unix)] -fn mode_copy_bits(mode: u32, copy_src: u32, copy_dst: u32, copy_and: u32) -> u32 { - let mut copy_bits = 0; - if copy_src & 0o100 != 0 { - copy_bits |= (mode >> 6) & 7; - } - if copy_src & 0o010 != 0 { - copy_bits |= (mode >> 3) & 7; - } - if copy_src & 0o001 != 0 { - copy_bits |= mode & 7; - } - (copy_dst * copy_bits) & copy_and -} - /// Core of `chmod.c:tweak_mode()`. /// /// `is_x` is sampled once from the original executable bits and `non_perm` @@ -64,9 +43,6 @@ fn tweak_mode(clauses: &[Clause], orig: u32, is_dir: bool) -> u32 { continue; } - // upstream: chmod.c - copy bits are resolved against the pre-AND mode. - let copy_bits = mode_copy_bits(mode, clause.copy_src, clause.copy_dst, clause.copy_and); - mode &= clause.mode_and; // upstream: chmod.c:229-232 - a conditional `X` only sets the execute @@ -76,15 +52,6 @@ fn tweak_mode(clauses: &[Clause], orig: u32, is_dir: bool) -> u32 { } else { mode |= clause.mode_or; } - - // upstream: chmod.c - a `-` copy clause clears the copied bits; every - // other operator sets them. Non-copy clauses have `copy_bits == 0`, - // leaving both branches a no-op. - if clause.is_sub { - mode &= CHMOD_BITS - copy_bits; - } else { - mode |= copy_bits; - } } mode | non_perm diff --git a/crates/metadata/src/chmod/comprehensive_tests.rs b/crates/metadata/src/chmod/comprehensive_tests.rs index 440fed3b2..85fa51081 100644 --- a/crates/metadata/src/chmod/comprehensive_tests.rs +++ b/crates/metadata/src/chmod/comprehensive_tests.rs @@ -182,14 +182,14 @@ mod basic_symbolic_syntax { assert!(!modifiers.is_empty()); } - // upstream: chmod.c:parse_chmod() STATE_2ND_HALF accepts a single who-letter - // on the RHS as a copy-from-category source. Mixing a copy letter with a - // literal permission letter in the same clause still routes to STATE_ERROR. - #[test] - fn who_letter_copy_source_in_rhs_accepted() { - assert!(ChmodModifiers::parse("g=u").is_ok()); - assert!(ChmodModifiers::parse("o=g").is_ok()); - assert!(ChmodModifiers::parse("o=u").is_ok()); + // upstream: chmod.c:159-185 STATE_2ND_HALF has no `u`/`g`/`o` case, so a + // category letter in the permission half routes to STATE_ERROR. rsync's + // `--chmod` grammar has no chmod(1)-style copy-from-category form. + #[test] + fn who_letter_copy_source_in_rhs_rejected() { + assert!(ChmodModifiers::parse("g=u").is_err()); + assert!(ChmodModifiers::parse("o=g").is_err()); + assert!(ChmodModifiers::parse("o=u").is_err()); assert!(ChmodModifiers::parse("g=ur").is_err()); } diff --git a/crates/metadata/src/chmod/mod.rs b/crates/metadata/src/chmod/mod.rs index f9f38fec4..1dda74834 100644 --- a/crates/metadata/src/chmod/mod.rs +++ b/crates/metadata/src/chmod/mod.rs @@ -5,12 +5,13 @@ //! The upstream rsync CLI allows multiple `--chmod=SPEC` occurrences where each //! specification may contain comma-separated numeric or symbolic clauses. This //! module mirrors upstream rsync's `chmod.c:parse_chmod()` grammar exactly, -//! reducing every clause to an AND/OR mask pair (`ModeAND`/`ModeOR`), the copy- -//! from-category transform (`ModeCOPY_*`), and the `D`/`F` selectors, then -//! applying them through `chmod.c:tweak_mode()` order: conditional execute bits -//! (`X`), the set-id/sticky bits driven by the who letters, the copy-from- -//! category distribution (`g=u`), and the umask masking applied to an implied -//! who-class. The [`ChmodModifiers`] type wraps the parsed clauses and exposes +//! reducing every clause to an AND/OR mask pair (`ModeAND`/`ModeOR`) plus the +//! `D`/`F` selectors, then applying them through `chmod.c:tweak_mode()` order: +//! conditional execute bits (`X`), the set-id/sticky bits driven by the who +//! letters, and the umask masking applied to an implied who-class. A category +//! letter (`u`/`g`/`o`) in the permission half is rejected exactly as upstream +//! does - rsync has no chmod(1)-style copy-from-category form. The +//! [`ChmodModifiers`] type wraps the parsed clauses and exposes //! [`ChmodModifiers::apply`] so higher layers can evaluate modifiers after the //! standard metadata preservation step. diff --git a/crates/metadata/src/chmod/parse.rs b/crates/metadata/src/chmod/parse.rs index 811b36acf..d7e394038 100644 --- a/crates/metadata/src/chmod/parse.rs +++ b/crates/metadata/src/chmod/parse.rs @@ -2,11 +2,12 @@ //! //! Faithful port of upstream rsync's `chmod.c:parse_chmod()` state machine. The //! whole modestring is scanned in a single pass so that comma handling, the -//! `D`/`F` selectors, octal modes, the symbolic `[ugoa][-+=][rwxXst]` forms, and -//! the copy-from-category forms (`[ugoa][-+=][ugo]`, e.g. `g=u`) match upstream -//! byte for byte, including the error transitions. Each clause is reduced to an -//! AND/OR pair plus a deferred copy-from-category transform consumed by the -//! evaluator in `apply.rs`. +//! `D`/`F` selectors, octal modes, and the symbolic `[ugoa][-+=][rwxXst]` forms +//! match upstream byte for byte, including the error transitions. A category +//! letter (`u`/`g`/`o`) in the second half is rejected exactly as upstream does +//! (chmod.c STATE_2ND_HALF `default:` -> STATE_ERROR): rsync's `--chmod` grammar +//! has no chmod(1)-style copy-from-category form. Each clause is reduced to an +//! AND/OR pair consumed by the evaluator in `apply.rs`. use super::{ChmodError, spec::CHMOD_BITS, spec::Clause}; @@ -55,25 +56,6 @@ fn orig_umask() -> u32 { 0 } -/// Returns the set-id/sticky bits implied by a copy-from-category destination. -/// -/// upstream: chmod.c `mode_dest_special_bits()` - a copy-from-category `=` -/// clause must also clear the destination class's top bit so the assignment -/// does not leave a stale setuid/setgid/sticky bit behind. -fn mode_special_bits(where_: u32) -> u32 { - let mut bits = 0; - if where_ & 0o100 != 0 { - bits |= 0o4000; - } - if where_ & 0o010 != 0 { - bits |= 0o2000; - } - if where_ & 0o001 != 0 { - bits |= 0o1000; - } - bits -} - /// Parses a `--chmod` specification against the process umask. pub(crate) fn parse_spec(modestr: &str) -> Result, ChmodError> { parse_with_umask(modestr, orig_umask()) @@ -92,7 +74,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result, let mut op = Op::None; let mut topbits: u32 = 0; let mut topoct: u32 = 0; - let mut copybits: u32 = 0; let mut x_keep = false; let mut dirs_only = false; let mut files_only = false; @@ -110,10 +91,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result, )); } - // upstream: chmod.c - the pre-fallback who selects whether copied - // bits are masked by ~umask (implied who) or kept whole. - let where_specified = where_; - // upstream: chmod.c:73-78. let bits = if where_ != 0 { where_ * what @@ -122,41 +99,21 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result, (where_ * what) & !umask }; - let copy_and = if where_specified != 0 { - CHMOD_BITS - } else { - !umask - }; - - // upstream: chmod.c:80-97 plus the ModeCOPY_* fields. - let (mode_and, mode_or, copy_src, is_sub) = match op { - Op::Add => (CHMOD_BITS, bits + topoct, copybits, false), - Op::Sub => (CHMOD_BITS - bits - topoct, 0, copybits, true), + // upstream: chmod.c:80-97. + let (mode_and, mode_or) = match op { + Op::Add => (CHMOD_BITS, bits + topoct), + Op::Sub => (CHMOD_BITS - bits - topoct, 0), Op::Eq => { let special = if topoct != 0 { topbits } else { 0 }; - let copy_special = if copybits != 0 { - mode_special_bits(where_) - } else { - 0 - }; - ( - CHMOD_BITS - (where_ * 7) - special - copy_special, - bits + topoct, - copybits, - false, - ) + (CHMOD_BITS - (where_ * 7) - special, bits + topoct) } - Op::Set => (0, bits, 0, false), + Op::Set => (0, bits), Op::None => unreachable!(), }; clauses.push(Clause { mode_and, mode_or, - copy_src, - copy_dst: if op == Op::Set { 0 } else { where_ }, - copy_and: if op == Op::Set { CHMOD_BITS } else { copy_and }, - is_sub, x_keep, dirs_only, files_only, @@ -175,7 +132,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result, op = Op::None; topbits = 0; topoct = 0; - copybits = 0; x_keep = false; dirs_only = false; files_only = false; @@ -235,29 +191,26 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result, } } }, - // upstream: chmod.c STATE_2ND_HALF. A literal permission set and a - // copy-from-category letter are mutually exclusive: once `copybits` - // is set, any `rwxXst` errors, and a `u`/`g`/`o` copy letter errors - // if any permission bit or a prior copy letter was already seen. + // upstream: chmod.c:159-185 STATE_2ND_HALF. Only `rwxXst` are valid; + // anything else (including a `u`/`g`/`o` category letter - rsync has + // no chmod(1)-style copy-from-category form) hits `default:` and + // routes to STATE_ERROR. State::SecondHalf => match byte { - b'r' if copybits == 0 => what |= 4, - b'w' if copybits == 0 => what |= 2, - b'X' if copybits == 0 => { + b'r' => what |= 4, + b'w' => what |= 2, + b'X' => { x_keep = true; what |= 1; } - b'x' if copybits == 0 => what |= 1, - b's' if copybits == 0 => { + b'x' => what |= 1, + b's' => { if topbits != 0 { topoct |= topbits; } else { topoct = 0o4000; } } - b't' if copybits == 0 => topoct |= 0o1000, - b'u' if what == 0 && topoct == 0 && copybits == 0 => copybits = 0o100, - b'g' if what == 0 && topoct == 0 && copybits == 0 => copybits = 0o010, - b'o' if what == 0 && topoct == 0 && copybits == 0 => copybits = 0o001, + b't' => topoct |= 0o1000, _ => return Err(err(c)), }, // upstream: chmod.c:187-194. @@ -404,48 +357,17 @@ mod tests { } #[test] - fn copy_from_category_eq_clause() { - // upstream: chmod.c STATE_2ND_HALF `u`/`g`/`o` set ModeCOPY_SRC; the `=` - // clause clears the destination class (and its top bit) then copies. - let c = one("g=u"); - assert_eq!(c.mode_and, CHMOD_BITS - 0o070 - 0o2000); - assert_eq!(c.mode_or, 0); - assert_eq!(c.copy_src, 0o100); - assert_eq!(c.copy_dst, 0o010); - assert_eq!(c.copy_and, CHMOD_BITS); - assert!(!c.is_sub); - } - - #[test] - fn copy_from_category_sub_clause() { - // upstream: chmod.c - a `-` copy clause records ModeOP == CHMOD_SUB so - // the evaluator clears the copied bits instead of setting them. - let c = one("g-o"); - assert_eq!(c.mode_and, CHMOD_BITS); - assert_eq!(c.copy_src, 0o001); - assert_eq!(c.copy_dst, 0o010); - assert!(c.is_sub); - } - - #[test] - fn copy_from_category_forms_accepted() { - // upstream: chmod.c accepts a single who-letter as the copy source for - // any of the +/-/= operators. - for spec in ["g=u", "o=g", "u+g", "a=u", "g-o", "o=u"] { - assert!(parse(spec).is_ok(), "`{spec}` should parse"); + fn copy_from_category_forms_rejected() { + // upstream: chmod.c:159-185 STATE_2ND_HALF has no `u`/`g`/`o` case, so a + // category letter in the permission half falls to `default:` -> + // STATE_ERROR and parse_chmod returns NULL. rsync's `--chmod` grammar + // does not implement chmod(1)'s copy-from-category form; the caller emits + // `Invalid argument passed to --chmod ()` and exits RERR_SYNTAX. + for spec in ["g=u", "o=g", "u+g", "a=u", "g-o", "o=u", "g=ur", "g=us"] { + assert!(parse(spec).is_err(), "`{spec}` must be rejected"); } } - #[test] - fn mixed_copy_and_perm_rejected() { - // upstream: chmod.c - a copy letter and a literal permission letter are - // mutually exclusive within one clause. - assert!(parse("g=ur").is_err()); - assert!(parse("g=ru").is_err()); - assert!(parse("g=uu").is_err()); - assert!(parse("g=us").is_err()); - } - #[test] fn empty_and_stray_commas_rejected() { // upstream: chmod.c:61-63 - a clause with no operator errors. diff --git a/crates/metadata/src/chmod/spec.rs b/crates/metadata/src/chmod/spec.rs index 7c69ab8ea..8a2442d41 100644 --- a/crates/metadata/src/chmod/spec.rs +++ b/crates/metadata/src/chmod/spec.rs @@ -13,25 +13,13 @@ pub(crate) const CHMOD_BITS: u32 = 0o7777; /// One parsed `--chmod` clause reduced to an AND/OR transform. /// /// upstream: chmod.c `struct chmod_mode_struct` fields `ModeAND`, `ModeOR`, -/// `ModeCOPY_SRC`, `ModeCOPY_DST`, `ModeCOPY_AND`, `ModeOP`, `flags`. +/// `flags`. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) struct Clause { /// Bits retained from the existing mode. upstream: `ModeAND`. pub(crate) mode_and: u32, /// Bits unconditionally set after masking. upstream: `ModeOR`. pub(crate) mode_or: u32, - /// Who-class to copy resolved perms from (`0o100`/`0o010`/`0o001`), or `0` - /// when the clause carries no copy-from-category letter. upstream: - /// `ModeCOPY_SRC`. - pub(crate) copy_src: u32, - /// Who-classes the copied perms are written into. upstream: `ModeCOPY_DST`. - pub(crate) copy_dst: u32, - /// Mask applied to the distributed copy bits (`CHMOD_BITS` for an explicit - /// who, `~umask` for the implied who). upstream: `ModeCOPY_AND`. - pub(crate) copy_and: u32, - /// Whether the clause operator is `-`, so copied bits are cleared instead of - /// set. upstream: `ModeOP == CHMOD_SUB`. - pub(crate) is_sub: bool, /// `X` conditional-execute flag. upstream: `FLAG_X_KEEP`. pub(crate) x_keep: bool, /// `D` selector: apply to directories only. upstream: `FLAG_DIRS_ONLY`. diff --git a/crates/metadata/src/chmod/tests.rs b/crates/metadata/src/chmod/tests.rs index 19350d7d0..834826654 100644 --- a/crates/metadata/src/chmod/tests.rs +++ b/crates/metadata/src/chmod/tests.rs @@ -61,48 +61,24 @@ fn conditional_execute_bit_behaviour_matches_rsync() { assert_eq!(dir_mode & 0o777, 0o711); } -/// upstream: chmod.c:parse_chmod() STATE_2ND_HALF accepts a single who-letter -/// (`u`/`g`/`o`) on the right-hand side as a copy-from-category source, and an -/// empty right-hand side clears that class. This is the reported `--chmod` -/// grammar that oc previously rejected. +/// upstream: chmod.c:159-185 STATE_2ND_HALF has no `u`/`g`/`o` case, so a +/// category letter in the permission half falls to `default:` -> STATE_ERROR +/// and parse_chmod returns NULL. rsync's `--chmod` grammar has no chmod(1)-style +/// copy-from-category form; upstream 3.4.4 prints +/// `Invalid argument passed to --chmod (g=u)` and exits RERR_SYNTAX. An empty +/// permission half (e.g. `o=`) is a distinct, legitimate clause and still +/// parses (the operator was seen, so it is not an empty clause). #[test] -fn who_letter_copy_forms_are_accepted() { - assert!(ChmodModifiers::parse("g=u").is_ok()); - assert!(ChmodModifiers::parse("g=o,o=").is_ok()); - assert!(ChmodModifiers::parse("g-o").is_ok()); - assert!(ChmodModifiers::parse("u+g").is_ok()); - // A copy letter mixed with a literal permission letter is still an error. +fn who_letter_copy_forms_are_rejected() { + assert!(ChmodModifiers::parse("g=u").is_err()); + assert!(ChmodModifiers::parse("o=g").is_err()); + assert!(ChmodModifiers::parse("u+g").is_err()); + assert!(ChmodModifiers::parse("g-o").is_err()); assert!(ChmodModifiers::parse("g=ur").is_err()); -} - -/// upstream: chmod.c copy-from-category apply. Resulting mode bits are verified -/// byte-for-byte against `rsync 3.4.3-149` on Linux (umask 022): -/// `g=u` 700->770, `g=o` 707->777, `u=g` 755->555, `g-o` 777->707, `o=` 755->750. -#[cfg(unix)] -#[test] -fn copy_from_category_apply_matches_rsync() { - use std::os::unix::fs::PermissionsExt; - - let temp = tempfile::tempdir().expect("tempdir"); - let file_path = temp.path().join("f"); - std::fs::write(&file_path, b"payload").expect("write file"); - std::fs::set_permissions(&file_path, PermissionsExt::from_mode(0o644)).expect("set perms"); - let file_type = std::fs::metadata(&file_path).expect("metadata").file_type(); - - let apply = |spec: &str, mode: u32| { - ChmodModifiers::parse(spec) - .expect("parse") - .apply(mode, file_type) - & 0o7777 - }; - - assert_eq!(apply("g=u", 0o700), 0o770); - assert_eq!(apply("g=u", 0o4755), 0o4775); - assert_eq!(apply("g=o", 0o707), 0o777); - assert_eq!(apply("u=g", 0o755), 0o555); - assert_eq!(apply("g-o", 0o777), 0o707); - assert_eq!(apply("o=", 0o755), 0o750); - assert_eq!(apply("a=u", 0o700), 0o777); + // An empty permission half clears the class and remains valid. + assert!(ChmodModifiers::parse("g=o,o=").is_err()); + assert!(ChmodModifiers::parse("o=").is_ok()); + assert!(ChmodModifiers::parse("Dg=").is_ok()); } /// Verifies that `D+w` (no explicit who) applies umask masking.