From ff7029a2eed40a76f4d8026793f17248b692695d Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 03:06:47 +0300 Subject: [PATCH 1/5] fix(generator): anchor the sender's directory scan on the transfer root A daemon pull of `rsync://host/module/cd/`, where `cd` is a symlink out of the module, enumerated the outside directory into the file list. Measured on the upstream 3.5.0 cell `sender-flist-symlink-leak`: the marker file outside the module appears in the listing, while the cell's own in-module positive control also passes, so the listing machinery is live and the leak is real. The confined enumeration was already routed. `walk.rs` calls `fast_io::pinned_root::read_dir`, which resolves its anchor through `confinement::pinned_root_relative` - the process-global session root. Nothing installs that root for a daemon, so `pinned_root_relative` returned `None`, which is documented to mean "resolve the path the ordinary way", and every directory read silently degraded to an unconfined absolute-path read. Meanwhile `GeneratorContext::confine_root()` was already correct for a daemon (`is_daemon_connection => daemon_module_root`, mirroring upstream's `am_daemon ? module_dir : confine_root`, syscall.c:136) and was consulted only by the CONTENT open. That split is exactly the observed behaviour: the content transfer refuses the escape while the enumeration follows it. The scan now takes the root as a PARAMETER rather than reading ambient state. `read_dir_under(root, path)` performs the same confined walk `read_dir` does for the pinned case, and `GeneratorContext::scan_source_dir` is the single owner of the anchoring decision for both scan sites, so they cannot disagree. Installing the global per connection was rejected deliberately: it is a `static RwLock` and oc serves each daemon connection on a worker thread of one process, so two concurrent connections on different modules would overwrite each other's boundary. Upstream's equivalent global is safe only because upstream forks a child per connection. Passing the root in is what makes the anchor correct under threads. Not-anchorable is not an escape. `strip_prefix` is lexical, so a relative operand - which a restricted shell produces routinely - or a differently spelled absolute path falls back to the ordinary read, matching what `pinned_root_relative` already decides for the ambient pin. Refusing there instead was measured to break `rrsync-pull-arg-shapes` and `rrsync-merge-file-confine` with `opendir "sub" failed: Cross-device link`, directories that were never outside anything. Measured on macOS against a freshly built release binary, whole suite: before 8 fail / 232 pass after 6 fail / 234 pass `sender-flist-symlink-leak` and `daemon-scan-dir-escape` both move fail -> pass; the remaining six are unchanged and all recorded `fail` in the manifest. Both macOS manifest rows are flipped in this commit because the defect is FIXED, not to silence a red - leaving them would report an XPASS. The two Linux manifests already record both cells as `pass` and are untouched. upstream: rsync-3.5.0/flist.c:2028-2059 `secure_opendir()` - the confined open that produces `scan_dirfd` for a daemon's scan. upstream: rsync-3.5.0/syscall.c:136 `confinement_root()` - `am_daemon ? module_dir : confine_root`, the value the new parameter carries. --- crates/fast_io/src/pinned_root.rs | 62 +++++++++++++++++++ .../transfer/src/generator/file_list/walk.rs | 36 ++++++++++- .../upstream-3.5.0-expect.macos.nonroot.txt | 4 +- 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/crates/fast_io/src/pinned_root.rs b/crates/fast_io/src/pinned_root.rs index 9ca9ef4b4..74bb46873 100644 --- a/crates/fast_io/src/pinned_root.rs +++ b/crates/fast_io/src/pinned_root.rs @@ -107,6 +107,68 @@ pub fn read_dir(path: &Path) -> io::Result { Ok(ReadDir(Source::Std(std::fs::read_dir(path)?))) } +/// Like [`read_dir`], but anchored on a root the CALLER supplies instead of the +/// ambient session pin. +/// +/// [`read_dir`] reads its anchor from the process-global session root, which is +/// only ever correct when the process serves one confinement domain at a time - +/// upstream's situation, because it forks a child per connection. oc serves +/// each daemon connection on a worker thread of one process, so a per-connection +/// root cannot live in a global without two concurrent connections overwriting +/// each other's boundary. Passing the root in makes the anchor a parameter of +/// the call rather than ambient state, which is what keeps it correct under +/// threads. +/// +/// The walk beneath `root` is the confined one: an absolute symlink target, a +/// `..` above the anchor, or an excluded component is REFUSED rather than +/// followed, so a refused scan can never be mistaken for an empty directory. +/// +/// ⚠ `strip_prefix` is LEXICAL, so "does not lie beneath `root`" here means +/// NOT ANCHORABLE, never "escapes the root". A relative operand resolves +/// against the process cwd - which is typically inside the root, since that is +/// how a restricted shell invokes rsync - and an absolute path can be spelled +/// differently from the root it is under. Both fall back to the ordinary read, +/// exactly as [`crate::confinement::pinned_root_relative`] already decides for +/// the ambient pin: "Returns `None` - meaning resolve `path` the ordinary way - +/// when no root is pinned, or when `path` does not lie beneath the pinned +/// root." +/// +/// Treating a failed `strip_prefix` as an escape and refusing would be the same +/// error this anchoring exists to fix: a lexical test standing in for a +/// resolution decision. It refuses directories that were never outside +/// anything - measured, as `opendir "sub" failed: Cross-device link`. +/// +/// # Errors +/// +/// - The walk's refusal (`ELOOP` for an absolute or escaping symlink target, +/// `..` above the anchor), or the underlying `opendir` error. +/// +/// # Upstream Reference +/// +/// - `rsync-3.5.0/flist.c:2028-2059` `secure_opendir()` - the confined open that +/// produces `scan_dirfd` for a daemon's directory scan. +/// - `rsync-3.5.0/syscall.c:136` `confinement_root()` - `am_daemon ? module_dir +/// : confine_root`, the value this parameter carries. +#[cfg(unix)] +pub fn read_dir_under(root: &Path, path: &Path) -> io::Result { + let Ok(relative) = path.strip_prefix(root) else { + return read_dir(path); + }; + // `strip_prefix` yields an empty path when `path` IS the root; the walk + // spells that directory `.`, matching what upstream's post-`change_dir` + // code uses for the same directory (`flist.c:2059`). + let relative = if relative.as_os_str().is_empty() { + Path::new(".") + } else { + relative + }; + let names = crate::confined_readdir::read_dir_confined(root, relative)?; + Ok(ReadDir(Source::Names { + dir: path.to_path_buf(), + names: names.into_iter(), + })) +} + /// Open `path` as an `O_PATH` descriptor, anchored on the pinned root when it /// applies. /// diff --git a/crates/transfer/src/generator/file_list/walk.rs b/crates/transfer/src/generator/file_list/walk.rs index c445102ab..9875db2ab 100644 --- a/crates/transfer/src/generator/file_list/walk.rs +++ b/crates/transfer/src/generator/file_list/walk.rs @@ -122,6 +122,38 @@ impl<'a> WalkScope<'a> { } impl GeneratorContext { + /// Enumerates a source directory, confined beneath this transfer's root + /// when it has one. + /// + /// Sole owner of the enumeration-anchor decision for the walk, so the two + /// scan sites cannot disagree about whether a directory read is confined - + /// the same reason [`Self::confine_root`] is the sole owner of the boundary + /// it reads. + /// + /// The root is passed to `fast_io` rather than left ambient. `read_dir`'s + /// own anchor is the process-global session pin, which oc never installs + /// for a daemon, so this walk silently degraded to an ordinary + /// absolute-path read while [`Self::confine_root`] - already daemon-correct + /// - was consulted only by the CONTENT open. That split is exactly what let + /// a module symlink be enumerated but not read. Installing the global per + /// connection is not the alternative: oc serves each connection on a worker + /// thread of one process, so a per-connection value in a global would race + /// between concurrent connections on different modules. + /// + /// # Upstream Reference + /// + /// - `rsync-3.5.0/flist.c:2028-2059` `secure_opendir()` - the confined open + /// producing `scan_dirfd` for a daemon's scan. + /// - `rsync-3.5.0/flist.c:1878` - the `opendir` failure diagnostic both call + /// sites report, unchanged by the anchoring. + fn scan_source_dir(&self, path: &Path) -> io::Result { + #[cfg(unix)] + if let Some(root) = self.confine_root() { + return fast_io::pinned_root::read_dir_under(&root, path); + } + fast_io::pinned_root::read_dir(path) + } + /// Pre-checks a top-level source entry and walks it if it exists. /// /// Returns `true` if the entry was processed (exists or was handled as a @@ -512,7 +544,7 @@ impl GeneratorContext { let should_recurse = metadata.is_dir() && self.config.flags.recursive && scope.descends_into_subdirs(); let dir_read = if should_recurse { - match fast_io::pinned_root::read_dir(&path) { + match self.scan_source_dir(&path) { Ok(entries) => Some(entries), Err(e) => { // upstream: flist.c:1878 - rsyserr(FERROR_XFER, errno, "opendir %s failed", ...) @@ -685,7 +717,7 @@ impl GeneratorContext { ) -> io::Result<()> { let entries = match opened { Some(entries) => entries, - None => match fast_io::pinned_root::read_dir(dir_path) { + None => match self.scan_source_dir(dir_path) { Ok(entries) => entries, Err(e) => { // upstream: flist.c:1878 - rsyserr(FERROR_XFER, errno, "opendir %s failed", ...) diff --git a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt index 765ea49df..4b5c81b1a 100644 --- a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt +++ b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt @@ -129,7 +129,7 @@ daemon-refuse-compress pass daemon-refuse-compress-threads-alias skip daemon-refuse-delete-alias pass daemon-scan-cwd-desync pass -daemon-scan-dir-escape fail +daemon-scan-dir-escape pass daemon-secrets-file-symlink skip daemon-size-arg-overflow pass daemon-standalone-detach skip @@ -320,7 +320,7 @@ scanner-batch-flag-mismatch pass scanner-daemon-log-checksum pass scanner-delete-delay-overread pass secure-relpath-validation pass -sender-flist-symlink-leak fail +sender-flist-symlink-leak pass sender-readlink-atfd pass sender-remove-source-relative-anchor pass sender-remove-source-root-anchor skip From 50f41eb365c4e2803983d2f8fc32cdf828e6c751 Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 03:14:38 +0300 Subject: [PATCH 2/5] fix(generator): rewrap a doc line so a leading hyphen is not read as a list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clippy::doc_lazy_continuation rejected the scan_source_dir doc: a wrapped prose line began with `- was consulted only by ...`, which markdown reads as a list bullet, making the three following lines lazy continuations of a list item. The hyphen-as-aside style is fine mid-line; the defect is purely where the wrap put it. Reworded to parentheses so no line begins with a hyphen unless it is a genuine list item - every other `/// - ` in this file is one. ⚠ This was invisible to a local workspace clippy run: crates/fast_io fails clippy on this host under rust 1.94.0 (7 errors in copy_file_range / sendfile / iocp_stub / confinement, none of them in this change), and a crate that does not compile under clippy means every DEPENDENT crate is never linted at all. "Zero hits in my files" from such a run is vacuous. Verified here by linting transfer with only those inherited lints allowed. --- crates/transfer/src/generator/file_list/walk.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/transfer/src/generator/file_list/walk.rs b/crates/transfer/src/generator/file_list/walk.rs index 9875db2ab..c50aa29b7 100644 --- a/crates/transfer/src/generator/file_list/walk.rs +++ b/crates/transfer/src/generator/file_list/walk.rs @@ -133,9 +133,10 @@ impl GeneratorContext { /// The root is passed to `fast_io` rather than left ambient. `read_dir`'s /// own anchor is the process-global session pin, which oc never installs /// for a daemon, so this walk silently degraded to an ordinary - /// absolute-path read while [`Self::confine_root`] - already daemon-correct - /// - was consulted only by the CONTENT open. That split is exactly what let - /// a module symlink be enumerated but not read. Installing the global per + /// absolute-path read, while [`Self::confine_root`] (already + /// daemon-correct) was consulted only by the CONTENT open. That split is + /// exactly what let a module symlink be enumerated but not read. + /// Installing the global per /// connection is not the alternative: oc serves each connection on a worker /// thread of one process, so a per-connection value in a global would race /// between concurrent connections on different modules. From f84059d868c94bd1eb4108677098cbf85cc01de8 Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 03:14:50 +0300 Subject: [PATCH 3/5] docs(daemon): a lexical collapse is not resolved containment Three sites claimed the daemon's operand resolvers CONFINE, on the strength of collapsing `.` and `..`: "cannot enumerate outside the module root" "the result cannot escape the module root, so there is nothing left to refuse" "the result is confined to the module root by construction" The collapse is a STRING operation. It contains exactly the shapes it folds - a traversing spelling and a host-absolute one - and says nothing about where the path RESOLVES, because a symlink is resolved by the kernel at syscall time and no string operation can see it. Upstream does not rely on the collapse alone either: it enters the module with change_dir(module_chdir, CD_NORMAL) and scans through the confined secure_opendir(). That over-claim is why an out-of-module directory reached by a module-root symlink was enumerated into the file list: a reader had been told this layer already confined, so the downstream scan was left anchored on ambient state. Each site now says what the collapse does prove, and names where resolved containment is actually enforced - the confined source open and the anchored directory scan. Four citations in the same blocks were verified against the pinned 3.5.0 tree and retargeted; two were verified correct and left alone. util1.c:804 -> :881 glob_expand_module() clientserver.c:992 -> :1059 change_dir(module_chdir, CD_NORMAL) flist.c:2338-2349 -> :2610-2621 send_file_list() dir/fn split main.c:1203-1204 -> io.c:1497 the glob_expand_module call site util1.c:1183 unchanged sanitize_path's `..` handling flist.c:2589-2594 unchanged DOTDIR_NAME --- .../client_args/path_resolution.rs | 33 ++++++++++++++----- .../client_args/server_config.rs | 18 ++++++---- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs b/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs index 82746f6fd..ccf9a1a2f 100644 --- a/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs +++ b/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs @@ -208,15 +208,30 @@ fn resolve_receiver_dest( /// Sub-paths containing `..`, and host-absolute sub-paths, are COLLAPSED /// against the module root rather than refused - see /// [`collapse_module_relative`]. A crafted `rsync://host/mod/../etc/...` URL -/// therefore resolves to `/etc/...` and cannot enumerate outside the -/// module root, which is the same containment upstream gets from -/// `sanitize_path` at depth 0. +/// therefore resolves to `/etc/...`, which is the same containment +/// upstream gets from `sanitize_path` at depth 0. +/// +/// ⚠ That collapse is LEXICAL, so it contains exactly the shapes it folds - +/// `.`, `..` and a host-absolute spelling - and nothing else. It is NOT a +/// statement about where the returned path RESOLVES: a symlink is resolved by +/// the kernel at syscall time and no string operation can see it. Upstream +/// does not rely on the collapse alone either; it also enters the module with +/// `change_dir(module_chdir, CD_NORMAL)` (clientserver.c:992) and scans +/// through the confined `secure_opendir()` (flist.c:2028-2059). +/// +/// Resolved containment for these operands is therefore owned by the sender, +/// not by this function: the content open goes through the confined source +/// open, and the file-list scan is anchored on the transfer's confinement root +/// rather than read by absolute path. Do not read this collapse as a reason to +/// leave a downstream operation unconfined - it was read that way once, and an +/// out-of-module directory reached by a module-root symlink was enumerated +/// into the file list. /// /// # Upstream Reference /// -/// - `util1.c:804 glob_expand_module()` - strips the module name from each arg -/// - `clientserver.c:992 change_dir(module_chdir, CD_NORMAL)` - relativises args -/// - `flist.c:2338-2349 send_file_list()` - `dir/fn` split per positional +/// - `util1.c:881 glob_expand_module()` - strips the module name from each arg +/// - `clientserver.c:1059 change_dir(module_chdir, CD_NORMAL)` - relativises args +/// - `flist.c:2610-2621 send_file_list()` - `dir/fn` split per positional fn resolve_sender_sources( module_path: &std::path::Path, client_args: &[String], @@ -236,8 +251,10 @@ fn resolve_sender_sources( } all_empty = false; // Collapse `.` and `..` exactly as upstream's `sanitize_path` does at - // depth 0 - see [`collapse_module_relative`]. The result cannot escape - // the module root, so there is nothing left to refuse. + // depth 0 - see [`collapse_module_relative`]. The result is LEXICALLY + // beneath the module root, so there is no traversing spelling left to + // refuse; where it RESOLVES is decided by the sender's confined open + // and its anchored directory scan, not here. let collapsed = collapse_module_relative(tail.trim_start_matches('/')); let trimmed = collapsed.as_str(); if trimmed.is_empty() { diff --git a/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs b/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs index 5873060d9..34cceb560 100644 --- a/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs +++ b/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs @@ -154,19 +154,25 @@ fn build_server_config( .cloned() .unwrap_or_default(); - // upstream: main.c:1203-1204 + util1.c:804 (glob_expand_module) - receivers + // upstream: io.c:1497 + util1.c:881 (glob_expand_module) - receivers // resolve their destination by joining the module path with the client's // module-relative tail (e.g. `upload/realdir/` -> module + `realdir/`). // Senders (pull requests) split each positional the same way so the - // sender's per-source `dir/fn` (flist.c:2338-2349) walks the requested + // sender's per-source `dir/fn` (flist.c:2610-2621) walks the requested // sub-tree instead of the entire module root. The original argv[0] is // always the module root; legacy tests that push straight into the module // root keep that behaviour. // Both resolvers are total: they collapse `..` the way upstream's - // `sanitize_path` does at depth 0, so the result is confined to the module - // root by construction and there is no "resolves outside module root" - // rejection to represent. Upstream has no such daemon error either - a - // traversing tail is rewritten and served (util1.c:1183). + // `sanitize_path` does at depth 0, so no traversing SPELLING survives and + // there is no "resolves outside module root" rejection to represent. + // Upstream has no such daemon error either - a traversing tail is + // rewritten and served (util1.c:1183). + // ⚠ Total is not confined. The collapse is a string operation and says + // nothing about where the path resolves; a symlink beneath the module root + // still points wherever it points. Resolved containment is enforced where + // the syscalls are issued - the confined source open and the file-list + // scan anchored on the transfer's confinement root - not by these + // resolvers. let positional_args: Vec = if role == ServerRole::Receiver { let dest = resolve_receiver_dest( std::path::Path::new(&module.path), From 50a0144534f1b4e32c3f3a3da054cc35766cb977 Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 04:28:15 +0300 Subject: [PATCH 4/5] fix(metadata): apply the set-group-ID mode chmod(2) would apply Upstream reaches every mode change through do_chmod() -> chmod(2) (syscall.c:761). oc anchors the same change on a walked parent dirfd for symlink-race safety, so it issues fchmodat(2) instead - and on macOS the two syscalls disagree about S_ISGID for an unprivileged caller. Measured on macOS 26.5, euid 501, caller owns the target, same inode and same mode word for both calls: requested chmod(2) fchmodat(2) 0755 - ok 0755 | S_ISUID ok -> 04755 ok -> 04755 0755 | S_ISGID ok -> 00755 EPERM 0755 | both ok -> 04755 EPERM The kernel silently MASKS a set-group-ID bit it will not grant when the request arrives through chmod, but REFUSES the identical request through fchmodat. Linux masks on both, so this is not observable there. Left unhandled that turned `--chmod=a+s` into a fatal EPERM (exit 23) for an ordinary user where upstream completes with exit 0. Measured against the real rsync 3.5.0 binary on the same fixture: upstream exits 0 and leaves drwsr-xr-x / -rwSr--r--; oc exited 23 and applied nothing. The refusal is now retried once with S_ISGID cleared - exactly the mode the platform's own chmod would have applied - so the anchor is kept and the resulting mode is byte-identical to upstream (verified: 4/4 entries match after the fix). The retry cannot mask a genuine permission error: a caller that does not own the target is refused for the setgid-free mode too, and that error propagates. macos-setgid-ordinary-mode-regression moves fail -> pass on the macOS upstream-3.5.0 leg (234/6 -> 235/5); its expect row is flipped in the same commit because the defect is fixed, not re-baselined. chmod-setid still fails on macOS and its `fail` row is deliberately UNCHANGED: that cell asserts both setuid and setgid are set, and the platform will not grant setgid to this caller at all - the real rsync 3.5.0 binary lands on the same setuid-only mode (-rwSr--r--) on this host, so the assertion is unsatisfiable here rather than an oc defect. --- .../dir_sandbox/at_syscalls/metadata_ops.rs | 52 ++++++++++++++++++- .../upstream-3.5.0-expect.macos.nonroot.txt | 2 +- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs b/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs index 806602a33..4bec42bd5 100644 --- a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs +++ b/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs @@ -278,7 +278,57 @@ pub fn secure_chmod_at(path: &Path, mode: u32, follow_symlinks: bool) -> io::Res .file_name() .ok_or_else(|| io::Error::from_raw_os_error(libc::EINVAL))?; let dirfd = crate::secure_open_dir(parent)?; - fchmodat(dirfd.as_fd(), leaf, mode, follow_symlinks) + chmodat_with_chmod_setgid_semantics(dirfd.as_fd(), leaf, mode, follow_symlinks) +} + +/// `fchmodat` with the set-group-ID semantics `chmod(2)` has on this platform. +/// +/// Upstream reaches every mode change through `do_chmod()` -> `chmod(2)` +/// (syscall.c:761). oc anchors the same change on a walked parent dirfd for +/// symlink-race safety, which means it issues `fchmodat(2)` instead - and the +/// two syscalls do NOT agree about `S_ISGID` for an unprivileged caller. +/// +/// MEASURED on macOS 26.5 (euid 501, caller owns the target), same inode and +/// same mode word for both calls: +/// +/// | requested | `chmod(2)` | `fchmodat(2)` | +/// |----------------------|--------------------|---------------| +/// | `0755` | - | ok | +/// | `0755 \| S_ISUID` | ok -> `04755` | ok -> `04755` | +/// | `0755 \| S_ISGID` | ok -> `00755` | **EPERM** | +/// | `0755 \| both` | ok -> `04755` | **EPERM** | +/// +/// So the kernel silently MASKS a set-group-ID bit it will not grant when the +/// request arrives through `chmod`, but REFUSES the identical request through +/// `fchmodat`. Linux masks on both, which is why this is not observable there. +/// +/// Left unhandled, that turns `--chmod=a+s` into a fatal `EPERM` for an +/// ordinary user on macOS where upstream completes with exit 0 - measured +/// against the real rsync 3.5.0 binary, which leaves `drwsr-xr-x`. +/// +/// This helper restores upstream's semantics WITHOUT giving up the anchor: +/// on refusal it retries the same anchored `fchmodat` once with `S_ISGID` +/// cleared, which is exactly the mode the platform's own `chmod` would have +/// applied. The retry cannot mask a genuine permission error - a caller that +/// does not own the target is refused for the setgid-free mode too, and that +/// error propagates. +/// +/// upstream: rsync-3.5.0 syscall.c:761 `do_chmod()` -> `chmod(2)`; rsync.c:658-668 +/// `set_file_attrs()` treats the chmod result as fatal for a non-symlink, which +/// is why the refusal must not reach it. +fn chmodat_with_chmod_setgid_semantics( + dirfd: BorrowedFd<'_>, + leaf: &OsStr, + mode: u32, + follow_symlinks: bool, +) -> io::Result<()> { + let setgid = u32::from(libc::S_ISGID); + match fchmodat(dirfd, leaf, mode, follow_symlinks) { + Err(error) if error.raw_os_error() == Some(libc::EPERM) && mode & setgid != 0 => { + fchmodat(dirfd, leaf, mode & !setgid, follow_symlinks) + } + other => other, + } } /// Path-based `chown(2)` / `lchown(2)` on `link_path` through the libc diff --git a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt index 4b5c81b1a..036cfadf0 100644 --- a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt +++ b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt @@ -200,7 +200,7 @@ links pass log-control-chars pass log-file-symlink skip longdir pass -macos-setgid-ordinary-mode-regression fail +macos-setgid-ordinary-mode-regression pass malicious-dot-dir-delete-scope skip malicious-dot-file-delete-scope skip malicious-sender-delete-scope pass From 7b0c6720c46b0bcb92fbc4ead881ca0e87ca2c4c Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 04:40:43 +0300 Subject: [PATCH 5/5] fix(fast_io): spell the setgid bit portably in the chmod helper `u32::from(libc::S_ISGID)` is a real widening on macOS, where `S_ISGID` is `u16`, but a no-op on Linux, where it is already `u32` - so Linux clippy rejects it as `useless_conversion`. An `as` cast fails the other way round, tripping `unnecessary_cast` on Linux. Drop the conversion entirely and name the POSIX-fixed literal, matching the existing idiom in crates/cli/src/frontend/progress/format/list.rs. A unit test pins the literal against `libc::S_ISGID`, widening both sides to `u64` so the comparison itself stays lint-clean on either platform. Value-identical to the previous code, so the measured macOS testsuite result (235 pass / 5 fail) is unchanged. --- .../dir_sandbox/at_syscalls/metadata_ops.rs | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs b/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs index 4bec42bd5..ff34f3889 100644 --- a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs +++ b/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs @@ -322,10 +322,13 @@ fn chmodat_with_chmod_setgid_semantics( mode: u32, follow_symlinks: bool, ) -> io::Result<()> { - let setgid = u32::from(libc::S_ISGID); + // The set-group-ID bit, spelled as the POSIX-fixed literal: `libc::S_ISGID` + // is `u16` on macOS but already `u32` on Linux, so neither `u32::from` nor + // an `as` cast is lint-clean on both platforms. + const SETGID: u32 = 0o2000; match fchmodat(dirfd, leaf, mode, follow_symlinks) { - Err(error) if error.raw_os_error() == Some(libc::EPERM) && mode & setgid != 0 => { - fchmodat(dirfd, leaf, mode & !setgid, follow_symlinks) + Err(error) if error.raw_os_error() == Some(libc::EPERM) && mode & SETGID != 0 => { + fchmodat(dirfd, leaf, mode & !SETGID, follow_symlinks) } other => other, } @@ -584,3 +587,17 @@ pub fn utimensat_via_sandbox_or_fallback( filetime::set_symlink_file_times(link_path, atime, mtime) } } + +#[cfg(test)] +mod tests { + /// `chmodat_with_chmod_setgid_semantics` spells the set-group-ID bit as a + /// literal because `libc::S_ISGID` has a different width per platform. Pin + /// the literal against libc so a wrong value cannot go unnoticed. + /// + /// Both sides widen to `u64`, which is a real conversion on every supported + /// platform - comparing in `u32` would be a no-op cast on Linux. + #[test] + fn setgid_literal_matches_libc() { + assert_eq!(u64::from(libc::S_ISGID), 0o2000); + } +}