Repository navigation
Resolve macOS system symlinks before the storage ancestor link assertion - #104
Conversation
On macOS ordinary roots such as the temp directory sit behind sanctioned system links (/var -> /private/var), so BlobStore::new rejected every temp-dir root with UnsafeEntry and the usd_process suite failed on any pristine macOS checkout while CI (Windows/Linux) stayed green. Canonicalise ancestor components before the link assertion there and assert the resolved directory instead; the supplied root itself and the behaviour on other platforms stay unchanged. The ancestor-link storage test now expects the store to bind to the resolved directory on macOS. (cherry picked from commit 7882d18)
The comments claimed only sanctioned system links are canonicalised, but the code resolves any ancestor link on macOS; a reviewer pointed out the test's own user-created link proves it. Say exactly that so the documented contract matches the behaviour. (cherry picked from commit d02fd16)
axisrow
left a comment
axisrow
left a comment
There was a problem hiding this comment.
Verdict: approved
Promotion of the fork macOS store-root fix, reviewed as a two-commit diff against upstream/master.
The relaxation is sound and correctly scoped:
absolute_directoryis only ever called with the store root (host configuration, never an asset path —BlobStore::with_limitandcheck_rootare the only callers), so the looser macOS ancestor rule does not touch any asset-derived path.- On macOS every non-final prefix is
fs::canonicalize-ed and then asserted a real directory bydirectory(symlink-metadata based), so a dangling or non-directory ancestor still fails; each later iteration re-canonicalises the whole grown prefix, so there is no gap between a resolved ancestor and its children. The supplied root itself keeps the strict no-link check on every platform. - Off-macOS behaviour is byte-for-byte the old reject-all rule (
cfg!(target_os = "macos")is a compile-time constant, both branches are portablestd). - Post-open integrity is preserved:
check_rootre-runs the walk and comparescanonicalize(root)against the storedcanonical_root, so an ancestor link retargeted after open is still caught — either by the resolved-directory assertion or by the canonical-equality check. The same holds under the pre-existing, documented TOCTOU disclaimer. ParentDircomponents are rejected before the walk, so the canonicalise branch can never be used to launder..traversal on macOS.- The test split asserts the new macOS acceptance and keeps the non-macOS
UnsafeEntryassertion; the honest "a user-created ancestor link passes too" limitation is stated in all three doc/comment sites.
Optional, not required: the macOS test could also assert store.canonical_root() equals the canonical target of the link to pin the "binds to the resolved directory" claim to the observable API; the shared canonical_root = canonicalize(root) code already guarantees it today.
|
Thank you for tracking down the macOS We would like to keep this open but not merge it in its current form. On macOS the change canonicalises every ancestor link of the store root, so a user-created symlink above the root now passes as well. The ancestor check exists precisely to refuse such links (see the module doc in Could you narrow it to one of these?
If #105 alone makes the macOS suite green, option 2 may mean this PR is not needed at all — please let us know what you find. |
|
Extends the fix tracked in #133 (mirror of axisrow#23): resolves macOS system symlinks before the storage ancestor link assertion. |
What this changes
Mirror of the fork's #24 (
axisrow/BeamCraft), rebased onto upstreammaster73c3597.Two commits, verbatim:
7882d18):the BlobStore's ancestor-link check canonicalises system symlink components (
/var→/private/var) on macOS before asserting the root is link-free; the root itself stayslink-free.
d02fd16).Without this, every
usd_processstore test fails on macOS, becausestd::env::temp_dir()sits behind the
/varsymlink. Upstreammasteris red on macOS for the same reason(verified 2026-10-20 on
fd6fde9and unchanged since).How you verified it
cargo clippy -p beamcraft-host --all-targets -- -D warningscargo test -p beamcraft-host(host storage suite) andcargo test -p beamcraft-cli --test usd_processon macOS with a canonicalTMPDIRProvenance (clean-room)
🤖 Generated with Claude Code
Issue tracking
Fixes #347