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

RFC: Implement a sandbox for environment variables and files #2391

Closed
wants to merge 2 commits into from

Conversation

jsgf
Copy link

@jsgf jsgf commented Apr 5, 2018

Allow rustc to be invoked with constraints on which environment variables may
be queried, and which files may be included.

Rendered

Companion PR: rust-lang/rust#49387

@jsgf jsgf changed the title Initial draft of env-sandboxing RFC env-sandboxing RFC Apr 5, 2018
@jsgf jsgf changed the title env-sandboxing RFC RFC: Implement a sandbox for environment variables and files Apr 5, 2018
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. labels Apr 6, 2018
These are processed in the order:
1. `--env-clear`
2. `--env-allow NAME`
3. `--env-set NAME=VAL`
Copy link
Contributor

@fbstj fbstj Apr 6, 2018

Choose a reason for hiding this comment

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

what's the benefit of this (rustc --env-set NAME=value) over NAME=value rustc --env-allow NAME other than brevity? is it specifically to stop propagation into sub-processes?

Copy link
Author

Choose a reason for hiding this comment

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

Several reasons:

  • You can use it to override the value of things that rustc (or the linker) itself uses, like PATH or LD_LIBRARY_PATH (among others)
  • You can just change rustc's command-line args, whereas manipulating the environment might need deeper changes to whatever's invoking rustc (possibly system-dependent changes)
  • Non-propagation isn't something that had occurred to me, but it is related to my first point

Copy link
Contributor

Choose a reason for hiding this comment

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

my question was more that my understanding is that running > NAME=value rustc --env-allow NAME is only overriding the value for that instantiation of rustc, so I don't understand what your first point is specifying? are you meaning that some point before/around your 'sandboxed' area is able to access the environment but then the majority is going to use the --env-allow value?

your option may also be more sensible for any shells/clients which don't support 'instantiation-local environment variables', but I think the RFC could do with more discussion of these points.

thanks!

Copy link
Author

Choose a reason for hiding this comment

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

I mean that when rustc itself is - say - invoking the linker, it may use $PATH to find the executable. If we want env!("PATH") to work at all, but not necessarily return the PATH that's been set up for rustc to be able to find its linker, then we need some way of allowing env!("PATH") and rustc's own runtime call to std::env::var("PATH") to be able to return distinct values.

process environment variable to be logically pasted into the source as a string
(or `Option` string).

`Rustc` has the following command-line options to allow the access to
Copy link
Contributor

@fbstj fbstj Apr 6, 2018

Choose a reason for hiding this comment

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

nit: capitalised rustc

@sfackler
Copy link
Member

sfackler commented Apr 6, 2018

rust-lang/cargo#3676 is sort of related.

@jsgf
Copy link
Author

jsgf commented Apr 6, 2018

@sfackler There seems to be a few use-cases in controlling the environment visible to build scripts.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/dev-tools -- I'm never sure who owns this sort of thing, but it's not clearly T-compiler. =)

@Centril
Copy link
Contributor

Centril commented Apr 11, 2018

@nikomatsakis so my reasoning for T-compiler was: "well, it makes changes to rustc" but that's not the best argument ever =P

@Manishearth
Copy link
Member

I think it's fine under devtools.

(I also like this RFC and do think we (the devtools team) should eventually think about sandboxing build processes in general)

@est31
Copy link
Member

est31 commented Apr 12, 2018

I also like this RFC and do think we (the devtools team) should eventually think about sandboxing build processes in general

ohhh that would be awesome ❤️ ❤️ ❤️

@Centril Centril added A-env Environment variable related proposals & ideas A-sandbox Sandboxing related proposals & ideas labels Nov 22, 2018
@nrc nrc added T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. and removed T-cargo Relevant to the Cargo team, which will review and decide on the RFC. labels Sep 25, 2019
@nrc
Copy link
Member

nrc commented Sep 26, 2019

We discussed this in the Cargo meeting today. The RFC doesn't have much content related to Cargo, given the early conversation, we've re-assigned to dev-tools (and the compiler team).

@jsgf is this RFC still something that would be useful for you?

@jsgf
Copy link
Author

jsgf commented Sep 26, 2019

@nrc Yes. My plan is to split this into two RFCs to handle environment and files separately, and probably focus on env first as it is more straightforward. It's on my TODO list, but pushing closer to the top.

@Manishearth
Copy link
Member

@jsgf should we close this in the meantime?

@jsgf
Copy link
Author

jsgf commented Sep 27, 2019

@Manishearth Yes, I suppose so.

@jsgf jsgf closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-env Environment variable related proposals & ideas A-sandbox Sandboxing related proposals & ideas T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants