Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable symlink checking for uncached Snapshot captures #8074

Conversation

@stuhood
Copy link
Member

commented Jul 19, 2019

Problem

As described in #7963: when we capture Snapshots synchronously via Scheduler.capture_snapshots, we do it without the benefit of caching or invalidation. But we still end up using a symlink-aware codepath, meaning that the restrictions we place on absolute and escaping symlinks are unnecessary. More generally, all awareness of symlinks is unnecessary in that case, as the only reason we track them is to enable correct invalidation of caches.

Solution

Add a symlink_aware: bool flag to PosixFS, and use it to prevent a consumer from observing the existence of a symlink via scandir. Use the flag while capturing Snapshots synchronously in Scheduler.capture_snapshots.

Additionally, make Metadata capture lazy in scandir, to avoid actually computing it for directories and links.

Result

Fixes #7963 and unblocks #8071.

@stuhood stuhood requested review from illicitonion, gshuflin and patliu85 Jul 19, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Commits are useful to review independently.

@stuhood

This comment was marked as resolved.

Copy link
Member Author

commented Jul 24, 2019

The rsc shards are failing to find a JDK that we happen to be putting on the path: triaging that in #8104.

@stuhood stuhood force-pushed the twitter:stuhood/disable-symliink-checking-for-uncached-captures branch from e586ea0 to 6351fd3 Jul 30, 2019

@stuhood stuhood force-pushed the twitter:stuhood/disable-symliink-checking-for-uncached-captures branch from 6351fd3 to 05e3dc5 Jul 31, 2019

@stuhood stuhood requested review from blorente and removed request for gshuflin Aug 1, 2019

@blorente
Copy link
Contributor

left a comment

looks good, thanks! Can we add a unit test in the Rust code, or an integration test in the Python side?

Show resolved Hide resolved src/rust/engine/fs/src/lib.rs Outdated

@blorente blorente requested a review from pierrechevalier83 Aug 1, 2019

@pierrechevalier83
Copy link
Contributor

left a comment

Looks good to me. One nitpick style comment.

Show resolved Hide resolved src/rust/engine/fs/src/lib.rs Outdated

@stuhood stuhood force-pushed the twitter:stuhood/disable-symliink-checking-for-uncached-captures branch from f3b3c15 to 0dc5390 Aug 1, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Only timeouts. Merging, and will bump timeouts for those tests in followups.

@stuhood stuhood merged commit ed2100d into pantsbuild:master Aug 1, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@stuhood stuhood deleted the twitter:stuhood/disable-symliink-checking-for-uncached-captures branch Aug 1, 2019

@stuhood stuhood added this to the 1.19.x milestone Aug 1, 2019

stuhood added a commit that referenced this pull request Aug 1, 2019

Disable symlink checking for uncached Snapshot captures (#8074)
### Problem

As described in #7963: when we capture Snapshots synchronously via `Scheduler.capture_snapshots`, we do it without the benefit of caching or invalidation. But we still end up using a symlink-aware codepath, meaning that the restrictions we place on absolute and escaping symlinks are unnecessary. More generally, _all_ awareness of symlinks is unnecessary in that case, as the only reason we track them is to enable correct invalidation of caches.

### Solution

Add a `symlink_aware: bool` flag to `PosixFS`, and use it to prevent a consumer from observing the existence of a symlink via `scandir`. Use the flag while capturing `Snapshots` synchronously in `Scheduler.capture_snapshots`.

Additionally, make `Metadata` capture lazy in `scandir`, to avoid actually computing it for directories and links.

### Result

Fixes #7963 and unblocks #8071.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.