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.

Sign up for GitHub

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

Jump to bottom

Sandboxing allows creating files in the directories of input files #7273

Open
jmmv opened this issue Jan 28, 2019 · 1 comment
Open

Sandboxing allows creating files in the directories of input files #7273

jmmv opened this issue Jan 28, 2019 · 1 comment
Labels
not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request
Milestone
sandboxfs

Comments

@jmmv
Copy link
Contributor

jmmv commented Jan 28, 2019

When sandboxing is enabled, an action is currently allowed to create new files in the directories that correspond to input files.

This is probably an accident of how symlink-based sandboxing works: the directories are created in the sandbox location and are never protected. But, for sandboxfs-based sandboxing, this requires extra code and incurs a significant runtime cost due to the need to create directories just in case an action might want to write to them.

I think this is a mistake: treating the location of input files as read-only is generally good practice (imagine building off a source tree on a read-only file system). Thus we should move towards stricter sandboxing. I have an upcoming change to do this in the sandboxfs variant, but we should probably do the same in the symlink variant for consistency reasons.

However, doing this change might break existing builds so it's not trivial to roll out (might need to go through the incompatible changes dance). So far I have encountered a single problem (fixed in f876830) though, so the risk is probably low.
@jmmv jmmv added type: feature request P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team labels Jan 28, 2019
bazel-io pushed a commit that referenced this issue Jan 28, 2019
@jmmv
Do not create writable directory trees for input files.
b0703d7 
The sandboxfs sandboxed spawn was creating writable file hierarchies
for all input files.  This should not be necessary if actions are
well-behaved.  Therefore, remove this for stricter sandboxing and to
improve sandboxfs-based performance by avoiding a significant number
of directory operations per action.

More specifically, this code was put in place to workaround an issue
with linking on macOS where a helper tool (make_hashed_objlist.py)
wanted to create symlinks in the directories where inputs lived.
I don't know why I concluded that the sandboxing code was wrong
instead of thinking that the tooling had to be fixed.  I have now
fixed the offending tools in unknown commit so we can do this
simplification.

Addresses part of #7273.

Tested: Built a relatively large iOS app with this change and the
only problem I found is the aforementioned one in
make_hashed_objlist.py.  It is possible that this strictness change
breaks other rules but the success of this build makes me relatively
confident that problems are not widespread.

RELNOTES: None.
PiperOrigin-RevId: 231242503
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@jmmv @weixiao-huang
Do not create writable directory trees for input files.
4e5b482 
The sandboxfs sandboxed spawn was creating writable file hierarchies
for all input files.  This should not be necessary if actions are
well-behaved.  Therefore, remove this for stricter sandboxing and to
improve sandboxfs-based performance by avoiding a significant number
of directory operations per action.

More specifically, this code was put in place to workaround an issue
with linking on macOS where a helper tool (make_hashed_objlist.py)
wanted to create symlinks in the directories where inputs lived.
I don't know why I concluded that the sandboxing code was wrong
instead of thinking that the tooling had to be fixed.  I have now
fixed the offending tools in unknown commit so we can do this
simplification.

Addresses part of bazelbuild#7273.

Tested: Built a relatively large iOS app with this change and the
only problem I found is the aforementioned one in
make_hashed_objlist.py.  It is possible that this strictness change
breaks other rules but the success of this build makes me relatively
confident that problems are not widespread.

RELNOTES: None.
PiperOrigin-RevId: 231242503
@jmmv jmmv added this to the sandboxfs milestone Mar 14, 2019
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Do not create writable directory trees for input files.
2b59227 
    The sandboxfs sandboxed spawn was creating writable file hierarchies
    for all input files.  This should not be necessary if actions are
    well-behaved.  Therefore, remove this for stricter sandboxing and to
    improve sandboxfs-based performance by avoiding a significant number
    of directory operations per action.

    More specifically, this code was put in place to workaround an issue
    with linking on macOS where a helper tool (make_hashed_objlist.py)
    wanted to create symlinks in the directories where inputs lived.
    I don't know why I concluded that the sandboxing code was wrong
    instead of thinking that the tooling had to be fixed.  I have now
    fixed the offending tools in unknown commit so we can do this
    simplification.

    Addresses part of bazelbuild/bazel#7273.

    Tested: Built a relatively large iOS app with this change and the
    only problem I found is the aforementioned one in
    make_hashed_objlist.py.  It is possible that this strictness change
    breaks other rules but the success of this build makes me relatively
    confident that problems are not widespread.

    RELNOTES: None.
    PiperOrigin-RevId: 231242503
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 16, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@sgowroji sgowroji closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 2023
@tjgq tjgq reopened this Feb 16, 2023
@tjgq tjgq removed the stale Issues or PRs that are stale (no activity for 30 days) label Feb 16, 2023
@sgowroji sgowroji added the not stale Issues or PRs that are inactive but not considered stale label Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request
Projects
None yet
Milestone
sandboxfs
Development

No branches or pull requests
3 participants
@jmmv @tjgq @sgowroji