Skip to content

fix(metadata): apply an incoming crtime of 0 instead of skipping it - #7250

Draft
oferchen wants to merge 2 commits into
masterfrom
fix/crtime-zero-guard
Draft

fix(metadata): apply an incoming crtime of 0 instead of skipping it#7250
oferchen wants to merge 2 commits into
masterfrom
fix/crtime-zero-guard

Conversation

@oferchen

@oferchen oferchen commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Problem

Pulling from an upstream rsync daemon with --crtimes, oc leaves the
destination's birth time alone where upstream stamps the value it was sent.
Measured by hand against a real upstream 3.4.4 daemon (use chroot = no), both
clients pulling the same module:

source birthtime            1614830767
upstream 3.4.4 client dest  0
oc dest                     1614830767   <- diverges

oc's 1614830767 is not a preserved crtime at all. oc never issues the
setattrlist call, so the destination keeps the birth time APFS gave it when
the file was written - clamped to the backdated mtime. It coincidentally equals
the source value, which is what made this look like a preservation difference
rather than a skipped write.

Cause

crates/metadata/src/apply/mod.rs:728 carried a conjunct upstream does not
have:

// upstream: rsync.c:615 - `if (crtimes_ndx && !(flags & ATTRS_SKIP_CRTIME))`
if options.crtimes() && entry.crtime() != 0 && !attrs_flags.skip_crtime() {

The cited upstream line has two conjuncts; oc had three. Upstream
(rsync.c:615-623) gates only on crtimes_ndx && !(flags & ATTRS_SKIP_CRTIME)
and then on a plain difference test, with no zero check anywhere:

if (crtimes_ndx && !(flags & ATTRS_SKIP_CRTIME)) {
    time_t file_crtime = F_CRTIME(file);
    if (sxp->crtime == 0)
        sxp->crtime = get_create_time(fname, &sxp->st);
    if (!same_time(sxp->crtime, 0L, file_crtime, 0L)) {
        if (do_setattrlist_crtime(fname, file_crtime) == 0)

Zero is a legitimate value on the wire, not a marker for "absent". Upstream's
get_create_time() returns 0 when the sender is a daemon running without chroot
(syscall.c, since 3.4.3), so every file list produced by such a daemon carries
crtime 0 - and upstream stamps it. oc treated it as "nothing to apply".

Why removing the conjunct is safe

The guard was presumably defending the ambiguity in
FileEntry::crtime(), which returns 0 both for a genuine epoch value and for
"no extras present". That case cannot reach this code:
protocol/src/flist/read/metadata.rs:108-117 produces Some on every branch
when preserve_crtimes is on -

let crtime = if self.preserve_crtimes {
    if flags.crtime_eq_mtime() { Some(mtime) } else { Some(read_varlong(reader, 4)?) }
} else { None };
  • and read/mod.rs:735 then calls set_crtime, populating extras. So
    options.crtimes() (the crtimes_ndx analogue) already excludes the absent
    case, exactly as upstream relies on. On the local-scan path a genuine on-disk
    crtime of 0 should be applied anyway, which is what upstream does.

Fix

Drop the conjunct so the condition matches the upstream line it cites, and
replace the comment with one that explains why zero is deliberately not special.

Verification

cargo fmt --all -- --check                                      clean
cargo clippy --locked --workspace --all-targets --all-features
  --no-deps -- -D warnings                                      clean
cargo nextest run -p metadata --all-features -E 'test(crtime)'
                                                                17 tests run: 17 passed

Both directions. Restoring only the production conjunct turns the behaviour test
red:

17 tests run: 16 passed, 1 failed
  FAIL incoming_zero_crtime_is_applied_rather_than_treated_as_absent

skip_crtime_still_suppresses_an_incoming_zero_crtime stays green under both,
which is correct - it pins the conjunct upstream does have, so it must be
insensitive to this change.

End-to-end, the same hand repro with the fix applied:

source birthtime            1614830767
upstream 3.4.4 client dest  0
oc dest                     0            <- matches upstream

Scope

This is the receiver-side half of what was one task. It is a fidelity fix with
no security dimension: oc's client is the receiver, and no daemon-side guard
applies to it.

The other half - oc's daemon lacking upstream 3.4.3's
am_daemon && !am_chrooted guard around get_create_time() /
do_setattrlist_crtime() - is tracked separately and is not addressed here.
That one is security-relevant (upstream's rationale is parent-symlink escape
under use chroot = no, which FSOPT_NOFOLLOW does not cover because it guards
only the final component) and is deliberately left for a decision.

Verified on macOS/aarch64; the new tests are cfg(target_os = "macos") because
that is where crtime is settable, matching the existing crtime tests in this
file. No CI (outage), so local verification only.

options.crtimes() is the negotiated fact, not "the user typed -N"

Worth stating explicitly, because it is what makes the deletion safe rather than
a trade of a fidelity bug for a data bug. The apply site and the flist decoder
read the same boolean, so they cannot disagree:

crates/transfer/src/lib.rs:738                        preserve_crtimes: config.flags.crtimes
crates/transfer/src/receiver/transfer/setup/context.rs:289  .preserve_crtimes(self.config.flags.crtimes)
crates/transfer/src/generator/context.rs:528          .with_preserve_crtimes(self.config.flags.crtimes)

And config.flags.crtimes is a session-wide negotiated fact, not a raw user
request: setup/mod.rs:145 calls require_crtimes_capability(), which aborts
the session with a protocol violation when the peer cannot carry crtime -

if preserve_crtimes && !compat_flags.contains(CompatibilityFlags::VARINT_FLIST_FLAGS) {
    return Err(protocol::protocol_violation(
        "Both rsync versions must be at least 3.2.0 for --crtimes.",
    ));
}

That is exactly upstream's crtimes_ndx semantics, which is set during compat
negotiation and aborts otherwise (compat.c). So when this flag is on, every
decoded entry carries a crtime by construction, and the removed conjunct could
only ever have suppressed a legitimate value.

Note for reviewers: master is currently red

Master is red independently of this change (#7236's own new test fails there -
redo does temp+rename under --inplace and mkstemp hits EACCES). That failure
will appear on this PR too. It is not caused by this change, which touches only
the crtime apply condition in crates/metadata, and is owned elsewhere.

@github-actions github-actions Bot added the bug Something isn't working label Aug 7, 2026
@oferchen
oferchen force-pushed the fix/crtime-zero-guard branch from 6eb3deb to f013fe5 Compare August 12, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant