Skip to content

chmod: support permission copy modes#973

Open
steadytao wants to merge 3 commits into
masterfrom
fix/chmod-permission-copy
Open

chmod: support permission copy modes#973
steadytao wants to merge 3 commits into
masterfrom
fix/chmod-permission-copy

Conversation

@steadytao
Copy link
Copy Markdown
Member

Fixes #678 by adding chmod-style permission copies to --chmod.

This allows symbolic modes such as g=o,o= where one permission class is copied into another before later operations are applied. The implementation keeps the existing static AND/OR handling for normal rwx modes and adds runtime copy handling for u, g and o on the right side.

Copy link
Copy Markdown
Member

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review by claude:

Thanks for tackling #678 — useful addition and the implementation is on the right track.

I reviewed the logic and it looks correct:

  • Traced --chmod=g=o,o= on mode 0647: copy og, then clear o ⇒ 0670 (rw-rwx---), matching the new test.
  • Backward compatible: a u/g/o letter in the second half of a clause was previously a hard parse error, so this only accepts input that used to be rejected — no existing --chmod usage changes.
  • mode_copy_bits() reads the source class from the pre-AND mode, which is the correct chmod(1) semantics, and the single-source-class restriction (e.g. g=uo is rejected) matches POSIX permcopy.

Two things to address before merge:

  1. Documentation. This adds user-visible --chmod syntax (g=o, g=u, …) but rsync.1.md isn't updated. Please document the permission-copy form in the --chmod section so it's discoverable.

  2. Test coverage. testsuite/chmod-option_test.py only exercises g=o,o=. Please also add:

    • the subtract path (e.g. g-o) — the only case that exercises the new ModeOP == CHMOD_SUB branch in tweak_mode();
    • a copy-from-user case (e.g. g=u);
    • a rejection case (e.g. --chmod=g=ur must error) to cover the new STATE_ERROR guards in parse_chmod().

Otherwise looks good — thanks again.

@steadytao steadytao force-pushed the fix/chmod-permission-copy branch from fd32894 to 9c7a1fa Compare June 6, 2026 04:24
@steadytao
Copy link
Copy Markdown
Member Author

Updated this with --chmod documentation plus extra coverage for g=u, g-o and rejection of mixed copy/static syntax such as g=ur.

Copy link
Copy Markdown
Member

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — Claude (Anthropic) + a second-opinion pass from OpenAI Codex.

We re-reviewed the permission-copy feature with a focus on security and cross-platform correctness. Most of it checks out: the rwx copy math is correct, the parser correctly rejects malformed mixed clauses (g=ur, u=go), mode_copy_bits() only touches the low rwx bits, the CHMOD_BITS - copy_bits subtraction can't underflow with parser-produced values, and we found no portability problems (the arithmetic stays within small mode-bit ranges). Docs + tests from the last round look good.

⚠️ Potential security / compat concern: special bits on copy = modes

The copy = assignments preserve the destination class's setuid/setgid/sticky bits, which differs from GNU chmod. Verified empirically against a rebuilt rsync:

start mode --chmod= rsync result GNU chmod
04755 (setuid) u=g 04555 — setuid kept 0555
02755 (setgid) g=u 02775 — setgid kept 0775
01750 dir (sticky) o=u 01757 — sticky kept 0757

Why it matters: an admin who uses a daemon incoming chmod (or --chmod) with chmod-style copy assignments to normalize untrusted uploads would still leave setuid/setgid/sticky set unless they also append u-s,g-s,o-t. Because the docs say these modes follow "the normal chmod(1) parsing rules", the divergence is also a compatibility surprise.

Caveat (maintainer's call): this is actually consistent with rsync's existing = behavior — rsync's u=rwx likewise does not clear setuid unless an s/t is given — so it isn't a regression the copy code introduced in isolation; the new syntax simply inherits rsync's long-standing = special-bit semantics. The real question is whether the new chmod(1)-style copy modes should match GNU here.

Possible fix: in the CHMOD_EQ handling, also clear the destination class's special bit (S_ISUID for u, S_ISGID for g, S_ISVTX for o), and add tests for the 04755/02755/01750 cases above.

No other issues found

  • Security: no parser-acceptance or bit-math issues beyond the above.
  • Cross-platform: none for Linux/BSD/macOS/Solaris/Cygwin. (Minor nit: chmod.c line ~87 could mask with ACCESSPERMS & ~orig_umask rather than a raw ~orig_umask.)

Reviewed by Claude with an empirical second pass by OpenAI Codex (built rsync and compared output to GNU chmod).

@steadytao steadytao force-pushed the fix/chmod-permission-copy branch from 9c7a1fa to b2fc338 Compare June 6, 2026 05:01
@steadytao
Copy link
Copy Markdown
Member Author

Adjusted this to match GNU chmod-style copy assignment for the reviewed special-bit cases. Copy = now clears the destination class special bit with coverage for setuid, setgid and sticky directory cases.

@tridge
Copy link
Copy Markdown
Member

tridge commented Jun 6, 2026

@steadytao should ensure the docs explain what we do with special-bits and say that this matches GNU chmod

@steadytao
Copy link
Copy Markdown
Member Author

@steadytao should ensure the docs explain what we do with special-bits and say that this matches GNU chmod

Ah yeah probably best to make that explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rsync --chmod doesn't fully support chmod(1) mode syntax

2 participants