Skip to content

fix(metadata): reject --chmod copy-syntax clauses like upstream #6561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
oferchen merged 1 commit into from
Jul 13, 2026
Merged

fix(metadata): reject --chmod copy-syntax clauses like upstream #6561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions crates/cli/src/frontend/execution/drive/metadata/compute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand Down
33 changes: 22 additions & 11 deletions crates/cli/src/frontend/tests/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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]
Expand Down
33 changes: 0 additions & 33 deletions crates/metadata/src/chmod/apply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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
Expand All @@ -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
Expand Down
16 changes: 8 additions & 8 deletions crates/metadata/src/chmod/comprehensive_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
13 changes: 7 additions & 6 deletions crates/metadata/src/chmod/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
138 changes: 30 additions & 108 deletions crates/metadata/src/chmod/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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<Vec<Clause>, ChmodError> {
parse_with_umask(modestr, orig_umask())
Expand All @@ -92,7 +74,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result<Vec<Clause>,
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;
Expand All @@ -110,10 +91,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result<Vec<Clause>,
));
}

// 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
Expand All @@ -122,41 +99,21 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result<Vec<Clause>,
(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,
Expand All @@ -175,7 +132,6 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result<Vec<Clause>,
op = Op::None;
topbits = 0;
topoct = 0;
copybits = 0;
x_keep = false;
dirs_only = false;
files_only = false;
Expand Down Expand Up @@ -235,29 +191,26 @@ pub(crate) fn parse_with_umask(modestr: &str, umask: u32) -> Result<Vec<Clause>,
}
}
},
// 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.
Expand Down Expand Up @@ -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 (<spec>)` 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.
Expand Down
Loading
Loading