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

Ensure TMPDIR is set for Xcode’s make #7283

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Ensure TMPDIR is set for Xcode’s make #7283

merged 1 commit into from
Apr 11, 2020

Conversation

claui
Copy link
Contributor

@claui claui commented Apr 5, 2020

Edit: changed title and final paragraph to reflect new modifications. Also changed embarrassing grammar in thank-you notice.

Edit 2: changed final paragraph to reflect that the code has been moved to gems.rb.

  • 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. No tests because /var/tmp may or may not be writable on a machine that runs the tests.
  • Have you successfully run brew style with your changes locally? Yes, and it worked for the first time in ages
  • Have you successfully run brew tests with your changes locally? Not applicable Edit: yes but --only=utils.

This fixes an issue where at least in Xcode 11.0, make uses
/var/tmp as a fallback for temporary files unless TMPDIR is set:

$ strings "$(xcrun -f make)" | grep -B 3 fopen
TMPDIR
/var/tmp/
GmXXXXXX
fopen (temporary file)

Given that Homebrew filtered TMPDIR, and the /var/tmp directory may not be writable for non-root users, this would cause Homebrew’s build environment to error out:

$ brew ruby -e 'puts ENV["TMPDIR"]; puts `: | make -f -`'
Ignoring bigdecimal-2.0.0 because its extensions are not built. Try: gem pristine bigdecimal --version 2.0.0
[…]
Ignoring zlib-1.1.0 because its extensions are not built. Try: gem pristine zlib --version 1.1.0

make: *** fopen (temporary file): Permission denied.  Stop.

In practice, this would break brew audit, brew style, and other
commands, which would run make to build native gem extensions.

This commit adds TMPDIR to Homebrew’s list of allowed environment
variables. It also adds unconditionally sets TMPDIR to ${HOMEBREW_TEMP}, which
is user-controlled but also guaranteed to be set with a fallback of /tmp, which takes precedence
over /var/tmp in case TMPDIR is not set in the user’s environment.

This commit sets TMPDIR to ${HOMEBREW_TEMP} in the gem environment, which mirrors the behaviour we already have in other places.
We choose HOMEBREW_TEMP because that’s user-controlled but also falls back to /tmp in case TMPDIR is not set in the user’s environment.

Thanks to Bo Anderson for helping find the bug.

@Bo98
Copy link
Member

Bo98 commented Apr 5, 2020

To copy what I said over Slack: if there is a particular reason that TMPDIR is filtered out then ENV["TMPDIR"] = HOMEBREW_TEMP before gems are installed is another possibility. I've not looked through brew's TMPDIR usage much.

@MikeMcQuaid
Copy link
Member

ENV["TMPDIR"] = HOMEBREW_TEMP before gems are installed is another possibility. I've not looked through brew's TMPDIR usage much.

I would prefer this method is taken. Seems nicer to use the value if set by users.

@claui
Copy link
Contributor Author

claui commented Apr 5, 2020

@MikeMcQuaid You mean we should leave TMPDIR untouched if it is already set, otherwise set to the value of HOMEBREW_TEMP?

@MikeMcQuaid
Copy link
Member

As in: I think we shouldn't whitelist the value but instead unconditionally set it to HOMEBREW_TEMP.

@claui claui changed the title Fix Xcode’s make by adding TMPDIR to env whitelist Ensure TMPDIR is set for Xcode’s make Apr 5, 2020
@claui
Copy link
Contributor Author

claui commented Apr 5, 2020

@MikeMcQuaid Done, thanks for your feedback!

if there is a particular reason that TMPDIR is filtered out then ENV["TMPDIR"] = HOMEBREW_TEMP before gems are installed is another possibility. I've not looked through brew's TMPDIR usage much.

@Bo98 I’m quite confident Homebrew doesn’t rely on TMPDIR being unset: most of the time where TMPDIR is referred to in Homebrew’s code base, it’s already being set to HOMEBREW_TEMP (example 1 2 3). That’s why I’d suggest to set TMPDIR in brew.sh, right next to where HOMEBREW_TEMP is set.

@MikeMcQuaid
Copy link
Member

@claui
Copy link
Contributor Author

claui commented Apr 11, 2020

As a result, what about instead just setting it where it's needed e.g. for gems:
[…]

On the fence about that. On macOS, removing TMPDIR from the environment is an anomaly in itself and may cause other hard-to-find issues than the one at hand.

But that’s just me speculating. So let’s set it in gems.rb, and revisit/reconsider in case the issue pops up again.

This fixes an issue where at least in Xcode 11.0, `make` uses
`/var/tmp` as a fallback for temporary files unless `TMPDIR` is set:

```
$ strings "$(xcrun -f make)" | grep -B 3 fopen
TMPDIR
/var/tmp/
GmXXXXXX
fopen (temporary file)
```

Given that Homebrew filtered `TMPDIR`, and the `/var/tmp` directory may
not be writable for non-root users, this would cause Homebrew’s
build environment to error out:

```
$ brew ruby -e 'puts ENV["TMPDIR"]; puts `: | make -f -`'
```

```
Ignoring bigdecimal-2.0.0 because its extensions are not built. Try: gem pristine bigdecimal --version 2.0.0
[…]
Ignoring zlib-1.1.0 because its extensions are not built. Try: gem pristine zlib --version 1.1.0
make: *** fopen (temporary file): Permission denied.  Stop.
```

In practice, this would break `brew audit`, `brew style`, and other
commands, which would run `make` to build native gem extensions.

This commit sets `TMPDIR` to `${HOMEBREW_TEMP}` in the gem environment, which
mirrors the behaviour we already have in other places.
We choose `HOMEBREW_TEMP` because that’s user-controlled but also falls
back to `/tmp` in case `TMPDIR` is not set in the user’s environment.

Thanks to Bo Anderson for helping find the bug.

CC: Bo Anderson <mail@boanderson.me>
@claui
Copy link
Contributor Author

claui commented Apr 11, 2020

@MikeMcQuaid @Bo98 Updated.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks @claui!

@MikeMcQuaid MikeMcQuaid merged commit 73df589 into Homebrew:master Apr 11, 2020
@claui claui deleted the fix-make-tmpfile branch April 27, 2020 16:32
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 2, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants