fix(metadata): apply an incoming crtime of 0 instead of skipping it - #7250
Draft
oferchen wants to merge 2 commits into
Draft
fix(metadata): apply an incoming crtime of 0 instead of skipping it#7250oferchen wants to merge 2 commits into
oferchen wants to merge 2 commits into
Conversation
oferchen
force-pushed
the
fix/crtime-zero-guard
branch
from
August 12, 2026 19:03
6eb3deb to
f013fe5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Pulling from an upstream rsync daemon with
--crtimes, oc leaves thedestination'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), bothclients pulling the same module:
oc's
1614830767is not a preserved crtime at all. oc never issues thesetattrlistcall, so the destination keeps the birth time APFS gave it whenthe 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:728carried a conjunct upstream does nothave:
The cited upstream line has two conjuncts; oc had three. Upstream
(
rsync.c:615-623) gates only oncrtimes_ndx && !(flags & ATTRS_SKIP_CRTIME)and then on a plain difference test, with no zero check anywhere:
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 carriescrtime 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-117producesSomeon every branchwhen
preserve_crtimesis on -read/mod.rs:735then callsset_crtime, populating extras. Sooptions.crtimes()(thecrtimes_ndxanalogue) already excludes the absentcase, 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
Both directions. Restoring only the production conjunct turns the behaviour test
red:
skip_crtime_still_suppresses_an_incoming_zero_crtimestays 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:
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_chrootedguard aroundget_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, whichFSOPT_NOFOLLOWdoes not cover because it guardsonly the final component) and is deliberately left for a decision.
Verified on macOS/aarch64; the new tests are
cfg(target_os = "macos")becausethat 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:
And
config.flags.crtimesis a session-wide negotiated fact, not a raw userrequest:
setup/mod.rs:145callsrequire_crtimes_capability(), which abortsthe session with a protocol violation when the peer cannot carry crtime -
That is exactly upstream's
crtimes_ndxsemantics, which is set during compatnegotiation and aborts otherwise (
compat.c). So when this flag is on, everydecoded 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
--inplaceandmkstemphits EACCES). That failurewill 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.