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

Filter all environment variables by default #932

Closed
MikeMcQuaid opened this issue Sep 11, 2016 · 18 comments
Closed

Filter all environment variables by default #932

MikeMcQuaid opened this issue Sep 11, 2016 · 18 comments

Comments

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Sep 11, 2016

Homebrew should filter all user environment variables and favour the system defaults by default when doing brew install (or friends) or brew test. This will avoid environment contamination breaking the from-source build.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 11, 2016

Like env -i?

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Sep 11, 2016

Somewhat, yeh. We'll need to obviously keep some variables around. This may end up being either a whitelist or sourceing the system defaults.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 11, 2016

login(1): HOME, SHELL, PATH, TERM, LOGNAME and USER. Everything except PATH is obvious, and PATH set through

eval `/usr/libexec/path_helper -s`

but path_helper is affected by /etc/paths and /etc/paths.d which could be modified by sysadmin.

Anything else?

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Sep 11, 2016

TMPDIR, PWD (needed for some commands), LANG, LOGNAME, EDITOR and obviously HOMEBREW_* stuff.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 11, 2016

TMPDIR and friends are already overwritten by brew (#817).
LANG and friends: should we really allow users to set these to arbitrary values during install and test?
EDITOR: why do you ever need it during install and test?
HOMEBREW_*: maybe they can all be handled prior to resetting env? Anyway, shouldn't be hard to whitelist.

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Sep 11, 2016

Agreed all around, then 👍

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Sep 11, 2016

I'm more thinking with LANG that things may 💥 with no value set.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 11, 2016

I'm more thinking with LANG that things may 💥 with no value set.

Yeah, we can probably set LC_ALL to C or en_US.UTF-8.

@apjanke
Copy link
Contributor

@apjanke apjanke commented Sep 14, 2016

👍

For LANG et al, setting LANG and clearing the LC_* should be sufficient, and more minimal. LANG must be set.

I suspect we should use the system's default <region>.UTF-8. Like @zmwangx, says, we probably don't want users setting these to arbitrary values; the installation processes might be sniffing them. And from my experience with Oh My Zsh, it's kinda common for users to set these to wacky things.

Setting it to C will break some things, because that invokes undefined/system-dependent behavior, and different languages and libraries deal with it differently.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 14, 2016

For LANG et al, setting LANG and clearing the LC_* should be sufficient, and more minimal. LANG must be set.

Why? LANG is just a fallback, and when LC_ALL is set you don't need a fallback because everything's covered. See locale(1).

Setting it to C will break some things, because that invokes undefined/system-dependent behavior

Isn't C the well-defined POSIX locale? But sure, I don't do a lot of locale-related stuff so if you're knowledgeable on that front I'll take your word for it.

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Sep 14, 2016

Isn't C the well-defined POSIX locale? But sure, I don't do a lot of locale-related stuff so if you're knowledgeable on that front I'll take your word for it.

It can also be interpreted as "don't support UTF-8". We now have MacOS.language that we can use to read the system default.

@zmwangx
Copy link
Member

@zmwangx zmwangx commented Sep 14, 2016

It can also be interpreted as "don't support UTF-8".

Good point.

We now have MacOS.language that we can use to read the system default.

👍

@apjanke
Copy link
Contributor

@apjanke apjanke commented Sep 16, 2016

Why? LANG is just a fallback, and when LC_ALL is set you don't need a fallback because everything's covered. See locale(1).

Viewed the other way, LANG is the primary definition, and the others are just overrides, and LC_ALL is an override for the overrides. It doesn't really matter; I just find a single LANG variable feels cleaner.

Isn't C the well-defined POSIX locale?

Yes, C is the well-defined POSIX locale, but not all characters (well, byte sequences, really) are well-defined under that locale. Specifically, only the "Portable Character Set" (basically, the characters for 7-bit ASCII, and their corresponding encodings) is defined under POSIX. Other character/encoding values are technically undefined, and will have system-dependent behavior. Most libraries and applications will treat them as opaque values and assume a single-byte encoding (basically, doing the C char thing or treating it as binary data), but that's not required by POSIX, and some implementations may differ. In particular, when ruby is running under C, it will treat byte values over 127 as invalid encodings and throw an error when encountering them. (I'd guess because Ruby comes from Japan, and most of the rest of Unix comes from America, where non-English encodings are less of a concern.)

The POSIX locale is a minimal locale intended for portability, not one that handles everything in some uniform way.

Yeah, I have done a lot of locale and character-encoding work. Trust me that this is a rabbit hole we could go down forever before getting back to Homebrew's env sanitization. 😄

Blaisorblade added a commit to Blaisorblade/brew that referenced this issue Oct 13, 2016
Multiple issues have been filed about this environment variable, hence
give a warning. Homebrew#932 might make this unnecessary when merged, but
hopefully this warning can be merged quickly.
@imax9000
Copy link

@imax9000 imax9000 commented Feb 4, 2017

Got bitten by (the lack of) this: TOP var leaked from my shell and broke the build of openssl@1.1 for me: https://gist.githubusercontent.com/anonymous/e0bdd43763bb19d6e1499e00de399842/raw/b2a695e9ad714316add792524d8d3a7aaa615c7d/03.make

@johnhawkinson
Copy link
Contributor

@johnhawkinson johnhawkinson commented Feb 20, 2017

Somehow it seems that PR #1753 never got referenced here, as a draft implementation of this.
So fixing that.
(I was directed here via Homebrew/homebrew-core#10133 but never saw #1753 was active)

It doe sort of look like there are some unresolved subtleties in the discussion here that the #1753 implementor should maybe address.

MikeMcQuaid added a commit that referenced this issue Feb 27, 2017
Suggested solution for 'Filter all environment variables by default #932' issue
@akvadrako
Copy link

@akvadrako akvadrako commented Jun 22, 2017

This would be quite nice to have. I recently encountered a formula that broke because I have LIBDIR set in my environment and it was rather confusing to debug.

@ilovezfs
Copy link
Contributor

@ilovezfs ilovezfs commented Jun 22, 2017

@akvadrako you can try it out

export HOMEBREW_ENV_FILTERING=1

@MikeMcQuaid
Copy link
Member Author

@MikeMcQuaid MikeMcQuaid commented Dec 8, 2017

This will be done by default in Homebrew 1.4.0 (#3529) and is done by default right now for anyone on the master branch. It's caused minimal breakage which is nice.

@ghost ghost removed the help wanted label Dec 10, 2017
@lock lock bot added the outdated label May 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants