Skip to content

security(macos): give archive deduplication a private, unpredictable scratch directory - #999

Merged
nahime0 merged 2 commits into
mainfrom
security/889-private-link-scratch
Sep 14, 2026
Merged

nahime0 merged 2 commits into
mainfrom
security/889-private-link-scratch

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Closes #889.

What was wrong

macOS whole-archive deduplication wrote its copies into <tmp>/elephc-link-dedup-<pid>, created with create_dir_all — which succeeds on a directory that already exists — and copied archives in with fs::copy, which follows a symlink at the destination.

On a multi-user machine another local user could predict or race the compiler's pid, pre-create that directory with permissive access, and plant a symlink named after a bridge archive. The copy would then truncate and overwrite the symlink's target with the compiler user's permissions. The path is reached whenever at least two whole-archived bridges need deduplication.

Fix

Guarantee How
unpredictable mkdtemp(3) — six characters of kernel-chosen randomness, no pid in the name
exclusive mkdtemp creates or fails; a planted directory cannot be adopted
owner-only mode 0700 from the moment it exists, with no window between creation and a permission fixup
no symlink follow copies go through OpenOptions::create_new (O_CREAT | O_EXCL), which refuses to follow a symlink and fails if anything already sits at the destination
fails closed no private scratch → the plan is returned unchanged and the link proceeds with the original archives

Failing closed is the right trade here: deduplication is an optimization, and a predictable scratch path is not an acceptable price for it.

create_new is the second lock on the same door — the private directory already makes a planted symlink unreachable — but the issue asks for it and it costs nothing.

Cleanup is unchanged: PreparedArchives::cleanup still removes the directory after the linker has consumed the plan.

Tests

  • scratch_is_unpredictable_private_and_fresh — two scratch paths differ, neither contains the pid, both are 0700 directories and empty
  • an_existing_destination_is_refused_not_followed — a symlink planted at a destination name is refused by create_new, and its target still holds its original bytes

cargo test --bin elephc (1903) passes; cargo build is warning-free.

Note: these live in the binary crate — src/linker is declared in src/main.rs, not src/lib.rs — so they run under cargo test --bin elephc, not --lib.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

…scratch directory

Closes #889.

macOS whole-archive deduplication wrote its copies into
`<tmp>/elephc-link-dedup-<pid>`, created with `create_dir_all` — which SUCCEEDS
on a directory that already exists — and copied archives in with `fs::copy`,
which follows a symlink at the destination.

On a multi-user machine another local user could predict or race the compiler's
pid, pre-create that directory with permissive access, and plant a symlink named
after a bridge archive. The copy would then truncate and overwrite the symlink's
target with the compiler user's permissions. The path is reached whenever at
least two whole-archived bridges need deduplication.

The scratch directory now comes from `mkdtemp(3)`: six characters of
kernel-chosen randomness, EXCLUSIVE creation so a planted directory cannot be
adopted, and mode `0700` from the moment it exists — no window between creation
and a permission fixup. Failure to obtain one FAILS CLOSED: the plan is returned
unchanged and the link proceeds with the original archives, because
deduplication is an optimization and a predictable scratch path is not an
acceptable price for it.

Archive copies are written through `OpenOptions::create_new` (`O_CREAT |
O_EXCL`), which refuses to follow a symlink and fails outright if anything
already sits at the destination. The private directory already makes a planted
symlink unreachable; this is the second lock on the same door.

Cleanup is unchanged — `PreparedArchives::cleanup` still removes the directory
after the linker has consumed the plan.

Tests cover both halves: two scratch paths differ, carry no pid, are `0700` and
empty; and a pre-existing destination (a symlink among them) is refused with its
target left untouched.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@github-actions github-actions Bot added area:triage No primary component could be inferred from changed paths. size:s Small pull request. type:triage Needs a conventional title or branch prefix before review. labels Sep 13, 2026
@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown

Greptile Summary

This PR secures macOS whole-archive deduplication and updates the copy path following the previous review.

  • Creates an unpredictable, exclusive, owner-only scratch directory with mkdtemp.
  • Opens destination archives exclusively with create_new to prevent symlink following.
  • Streams archive contents with bounded memory instead of loading each archive in full.
  • Adds regression tests for scratch-directory properties and refusal of existing destinations.

Confidence Score: 5/5

The PR appears safe to merge; the previously reported whole-archive memory spike has been fully addressed by streaming the source into the exclusive destination.

No blocking or non-blocking actionable issues remain. The prior Greptile thread is manually resolved, and the current implementation replaces the full-file read with std::io::copy while preserving exclusive destination creation.

Important Files Changed

Filename Overview
src/linker/archive_dedup.rs Replaces predictable scratch storage and symlink-following copies with private exclusive creation, bounded-memory streaming, fail-closed behavior, and focused regression tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Link plan] --> B{Deduplication needed?}
    B -- No --> C[Return unchanged plan]
    B -- Yes --> D{Create private scratch directory}
    D -- Failure --> C
    D -- Success --> E[Open source archive]
    E --> F{Exclusively create destination}
    F -- Failure --> G[Keep original archive]
    F -- Success --> H[Stream archive contents]
    H --> I[Strip duplicate members]
    I --> J[Replace archive in prepared plan]
    J --> K[Linker consumes plan]
    K --> L[Cleanup scratch directory]
Loading

Reviews (2): Last reviewed commit: "perf(linker): stream the deduplication c..." | Re-trigger Greptile

Comment thread src/linker/archive_dedup.rs Outdated
@Guikingone Guikingone self-assigned this Sep 13, 2026
…archive

Review follow-up on #889: the exclusive `create_new` destination was being fed
from `fs::read`, which materializes the ENTIRE source archive in memory before a
byte is written. A whole-archived bridge runs to tens of megabytes and a macOS
link deduplicates several of them in sequence, so the peak was proportional to
the archive set for no reason at all.

`io::copy` streams it instead — the same thing `fs::copy` does internally, minus
the symlink-following open that made `fs::copy` unusable here in the first place.
The `O_CREAT | O_EXCL` destination, which is the security property #889 added, is
untouched: only the source side changes.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@nahime0

nahime0 commented Sep 14, 2026

Copy link
Copy Markdown
Member

@Guikingone is this ready? for me can be merged, but you didn't ask for my review :) let me know if I can merge it

@Guikingone

Guikingone commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator Author

Ah yes, forgot about it 😄

Ready for review / merge 🙂

@Guikingone
Guikingone requested a review from nahime0 September 14, 2026 15:56

@nahime0 nahime0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nahime0
nahime0 merged commit 4eb6448 into main Sep 14, 2026
148 checks passed
@nahime0
nahime0 deleted the security/889-private-link-scratch branch September 14, 2026 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:triage No primary component could be inferred from changed paths. size:s Small pull request. type:triage Needs a conventional title or branch prefix before review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(macos): use private unpredictable scratch space for archive deduplication

2 participants