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

brew.sh: Don't allow system tmp dirs as prefixes #4397

Merged
merged 14 commits into from Jul 3, 2018

Conversation

Projects
None yet
3 participants
@woodruffw
Copy link
Member

woodruffw commented Jun 30, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This explicitly forbids the use of the system's temporary directory as an installation prefix, as we perform checks against the temporary directory when doing binary relocation.

See: Homebrew/homebrew-core#28877 (comment)

@woodruffw woodruffw requested review from mistydemeo and ilovezfs Jun 30, 2018

@wafflebot wafflebot bot added the in progress label Jun 30, 2018

woodruffw added some commits Jun 30, 2018

@woodruffw woodruffw referenced this pull request Jun 30, 2018

Closed

openssl fails to build in non-standard prefix #28877

5 of 6 tasks complete
Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew
uses to store downloads and builds. You can resolve this by installing Homebrew to
either the standard prefix (/usr/local/) or to a non-standard prefix that is not
the system temporary directory.

This comment has been minimized.

@ilovezfs

ilovezfs Jun 30, 2018

Contributor

s/the/in the/

if [[ $(realpath "${HOMEBREW_PREFIX}") = /private/tmp/* ]]
then
odie <<EOS
Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 30, 2018

Member

Maybe worth just saying e.g. /private/tmp? Also, should we do this on Linux or with a different directory?

This comment has been minimized.

@woodruffw

woodruffw Jun 30, 2018

Member

I thought that the connection between /tmp and /private/tmp might confuse some users (Apple links /tmp to /private/tmp for FHS compliance), so I just went with "system temporary directory." I can change it/add those directories to the message if you think it'll improve the error, though.

The relocation code at fault isn't used on Linux, so Linuxbrew users might not encounter this problem at all. I'll confirm that in a bit with some local testing.

odie <<EOS
Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew
uses to store downloads and builds. You can resolve this by installing Homebrew to
either the standard prefix (/usr/local/) or to a non-standard prefix that is not

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 30, 2018

Member

(/usr/local) maybe?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 30, 2018

The underlying issue specifically affects HOMEBREW_TEMP regardless of whether it's the default /tmp or some other path (and if HOMEBREW_TEMP isn't /tmp then /tmp is unaffected) so I'm wondering if the check and the messaging should check the value of HOMEBREW_TEMP for a non-default value and handle that case correctly too.

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Jun 30, 2018

so I'm wondering if the check and the messaging should check the value of HOMEBREW_TEMP for a non-default value and handle that case correctly too.

Yeah, I'll update it to handle this. It gets a little complicated, though, thanks to the /tmp <-> /private/tmp link. Would anybody mind if I added a realpath to the assignment of HOMEBREW_TEMP in config.rb, so that we don't have to worry about handling all of the possible permutations of users installing in /tmp and /private/tmp?

Edit: Actually, I guess HOMEBREW_TEMP doesn't even exist in brew.sh's environment, since we don't set it until config.rb. In that case, I'll set it (with fallback) in bin/brew.

woodruffw added some commits Jun 30, 2018

bin/brew Outdated
@@ -72,7 +73,7 @@ then

FILTERED_ENV=()
# Filter all but the specific variables.
for VAR in HOME SHELL PATH TERM COLUMNS LOGNAME USER CI TRAVIS SSH_AUTH_SOCK SUDO_ASKPASS \
for VAR in HOME SHELL PATH TEMP TERM COLUMNS LOGNAME USER CI TRAVIS SSH_AUTH_SOCK SUDO_ASKPASS \

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

Why do we want to add this?

This comment has been minimized.

@woodruffw

woodruffw Jul 1, 2018

Member

Oh, I misread this code. I thought this was explicitly whitelisting HOMEBREW_TEMP, but I guess it would clobber it instead.

@@ -39,7 +39,7 @@
HOMEBREW_LOGS = Pathname.new(ENV["HOMEBREW_LOGS"] || "~/Library/Logs/Homebrew/").expand_path

# Must use /tmp instead of $TMPDIR because long paths break Unix domain sockets
HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp"))
HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp")).realpath

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

This will never be unset/set to /tmp now.

woodruffw added some commits Jul 1, 2018

config: Remove /tmp fallback
We provide a /private/tmp fallback in bin/brew, so
this is no longer necessary.
@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Jul 1, 2018

Looks like the test failure is coming from brew update-test, which uses HOMEBREW_TEMP via mktemp to test brew update.

Any thoughts on how best to resolve this? I could add another environment variable allowing the behavior in the PR to be overridden for testing purposes, or change brew update-test to avoid HOMEBREW_TEMP, off the top of my head. Neither of those is particularly clean, though 😞

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 1, 2018

change brew update-test to avoid HOMEBREW_TEMP

I'd vote on doing this; no reason it can't act in the current directory (as a developer, testing command) if it's cleaned up afterwards.

bin/brew Outdated
@@ -21,6 +21,7 @@ symlink_target_directory() {
BREW_FILE_DIRECTORY="$(quiet_cd "${0%/*}/" && pwd -P)"
HOMEBREW_BREW_FILE="${BREW_FILE_DIRECTORY%/}/${0##*/}"
HOMEBREW_PREFIX="${HOMEBREW_BREW_FILE%/*/*}"
HOMEBREW_TEMP="${HOMEBREW_TEMP:-/private/tmp}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

Will this upset Linux? Worth setting in brew.sh to a different default for macOS and not-macOS?

This comment has been minimized.

@woodruffw

woodruffw Jul 1, 2018

Member

Yeah, /private/tmp is macOS only. Moving it to brew.sh sounds good.

brew.sh: Move HOMEBREW_TEMP declaration
Additionally, assign HOMEBREW_TEMP based on the host
system (/tmp for Linux, /private/tmp for macOS).
@@ -108,6 +108,8 @@ then
then
HOMEBREW_CACHE="$HOME/Library/Caches/Homebrew"
fi

HOMEBREW_TEMP="${HOMEBREW_TEMP:-/private/tmp}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

👍 to defining here. I wonder if it's worth doing just the same -z as above for consistency? Alternatively; replace the -zs that have no complex logic with this form?

This comment has been minimized.

@woodruffw

woodruffw Jul 1, 2018

Member

I'm in favor of replacing with the :- form, since it's the standard bashism for a default value and we use similar bashisms elsewhere in the code.

@@ -294,6 +299,19 @@ EOS
}
check-run-command-as-root

check-prefix-is-not-tmpdir() {
if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_TEMP}"* ]]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

This wants guarded to macOS only, check against the system temp rather than just HOMEBREW_TEMP (maybe a HOMEBREW_SYSTEM_TEMP var?) and use that directory in the output, too.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 1, 2018

Member

HOMEBREW_SYSTEM_TEMP could be potentially used wherever private/tmp is hardcoded, too.

This comment has been minimized.

@woodruffw

woodruffw Jul 1, 2018

Member

check against the system temp rather than just HOMEBREW_TEMP

I might be misunderstanding, but: won't this leave open the case where they set both their prefix and their temp to something weird, e.g., $HOME? We want to stop all situations where Homebrew is both installing to and using the same prefix as a temporary directory, not just /private/tmp.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 2, 2018

Member

@woodruffw Ok so is it fine to e.g. have the prefix in /tmp if HOMEBREW_TEMP is set elsewhere?

This comment has been minimized.

@woodruffw

woodruffw Jul 2, 2018

Member

Yep, exactly.

woodruffw added some commits Jul 1, 2018

HOMEBREW_CACHE="$HOME/.cache/Homebrew"
fi
fi
cache_home="${XDG_CACHE_HOME:-${HOME}/.cache}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 2, 2018

Member

Use all caps for variable name here and just don't export it.

if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_TEMP}"* ]]
then
odie <<EOS
Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 2, 2018

Member

HOMEBREW_TEMP isn't necessary the system temporary directory. To expand on my previous comment (and avoid the autohiding): is the issue if:

  • the prefix is in the system temporary directory
  • the prefix is the in the HOMEBREW_TEMP temporary directory
  • either

We should ensure that the wording makes that clear and the logic is as minimal as possible to the actual problems we've seen people experience.

This comment has been minimized.

@woodruffw

woodruffw Jul 2, 2018

Member

The issue is when the prefix is in the HOMEBREW_TEMP temporary directory. I'll correct the language here 🙂

@MikeMcQuaid MikeMcQuaid merged commit 09ee556 into Homebrew:master Jul 3, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codecov/patch 72.72% of diff hit (target 63.16%)
Details
codecov/project 69.74% (+6.57%) compared to fac11c2
Details

@wafflebot wafflebot bot removed the in progress label Jul 3, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 3, 2018

Thanks @woodruffw and sorry for all the back and forth! Took me a while to understand what was going on here.

@@ -39,7 +39,7 @@
HOMEBREW_LOGS = Pathname.new(ENV["HOMEBREW_LOGS"] || "~/Library/Logs/Homebrew/").expand_path

# Must use /tmp instead of $TMPDIR because long paths break Unix domain sockets
HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp"))
HOMEBREW_TEMP = Pathname.new(ENV["HOMEBREW_TEMP"]).realpath

This comment has been minimized.

@ilovezfs

ilovezfs Jul 3, 2018

Contributor

I think this will crash when the dir doesn't exist.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 3, 2018

Member

Yup. Fixing 😭

This comment has been minimized.

@MikeMcQuaid

This comment has been minimized.

@ilovezfs

ilovezfs Jul 3, 2018

Contributor

Thanks!

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Jul 3, 2018

No problem at all, but: I hadn't applied the brew update-test changes yet, so this still breaks the CI. Maybe worth a revert (or I could just fill it in with another PR)?

(It's 100% my fault -- I forgot to add the "do not merge" label).

Ignore that -- I see #4415 takes care of the brew update-test change too. Thanks @MikeMcQuaid!

@woodruffw woodruffw deleted the woodruffw-forks:forbid-temp-prefix branch Jul 3, 2018

@lock lock bot added the outdated label Aug 2, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.