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

Jump to bottom

Purity checking should also accept $TMP, $TMPDIR and $TEMP, $TEMPDIR #97597

Merged
merged 1 commit into from Sep 24, 2020
Merged

Purity checking should also accept $TMP, $TMPDIR and $TEMP, $TEMPDIR #97597

merged 1 commit into from Sep 24, 2020

Conversation

twhitehead
Copy link
Contributor

Motivation for this change

@FRidh @jtojnar @matthewbauer @vcunat @pbogdan have to submit a new pull request for an update to #93560 as it was accepted (and hence closed) but later reverted.

The details again are, the badPath routine used by the various compiler paths to check purity is hard coded to reject any paths that lie outside of

  • $NIX_STORE,
  • $NIX_BUILD_TOP, and
  • /tmp.

In build environments, however, /tmp is frequently overriden by settings the $TMP, $TMPDIR, and $TEMP environment variables, so I submitted a pull request #93560 to make the system use $TMP, if set, instead of /tmp.

This broke packages that were hardcoded to use /tmp (e.g., mktemp /tmp/FOOXXXX). In retrospect, the patch was also somewhat lacking as it should technically should also have accept $TMPDIR and $TEMP in addition to $TMP. This is an updated pull request that accepts all of

  • $NIX_STORE,
  • $NIX_BUILD_TOP,
  • /tmp,
  • $TMP,
  • $TMPDIR, and
  • $TEMP.

Being strictly more accepting than the previous one, this one should not causes any failures.

Things done

I have verified the above GNU helloworld (see above) compiles correctly in the above example once this change is made. This is actually quite extensive as change the badPath routine does result in a fairly significant rebuild of dependencies.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@vcunat
Copy link
Member

vcunat commented Sep 9, 2020 •
edited

A quick thought: if we go for such a long positive list, wouldn't it be easier to just reject /usr/** and /lib/**? EDIT: perhaps it would be less clean.

@twhitehead
Copy link
Contributor Author

twhitehead commented Sep 10, 2020 •
edited

I agree with the more clean. If you have a specific list of acceptable paths, an exact whitelist seems like a better filter than an approximate blacklist to me.

It is unfortunate that a temporary location is a bit messy, but $TMP, $TMPDIR, and $TEMP are all the ones set by nix-shell and the likes. Actually, when I went to double checked this, I noted that the correct list is $TMP, $TMPDIR and $TEMP, $TEMPDIR. I'll update the patch

[nix-shell:~]$ env | egrep 'T[^=]*MP[^=]*='
...
TMP=/run/user/1000
TMPDIR=/run/user/1000
TEMPDIR=/run/user/1000
TEMP=/run/user/1000

In the original patch I just did $TMP as I assumed it was representative of the lot of them, but I figured making it exact was probably better to avoid any possibility of a corner case at some point. I can switch it back to just $TMP if people want. The point of this patch was to include /tmp regardless of the setting of any environment variables as it seems not all programs/build scripts honour them.

@twhitehead twhitehead changed the title Purity checking should also accept $TMP, $TMPDIR, and $TEMP Purity checking should also accept $TMP, $TMPDIR and $TEMP, $TEMPDIR Sep 10, 2020
@vcunat vcunat mentioned this pull request Sep 18, 2020
Closed
10 tasks
@vcunat vcunat changed the base branch from master to staging September 20, 2020 16:07
vcunat
vcunat approved these changes Sep 20, 2020
View reviewed changes
Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

Seems good to me. The conflict is due to the previous version of this getting reverted.

I believe this can be merged to staging without further testing. This new version "breaks" only a subset of badPath invocations in comparison to the previous version which turned out not to be that bad (was in master and release-20.09 for some time).

@vcunat vcunat mentioned this pull request Sep 21, 2020
Closed
@vcunat vcunat merged commit a8cda11 into NixOS:staging Sep 24, 2020
This was referenced Oct 4, 2020
Closed
Closed
@risicle risicle mentioned this pull request Oct 17, 2020
Merged
10 tasks
risicle added a commit to risicle/nixpkgs that referenced this pull request Oct 17, 2020
@risicle
python3Packages.Theano: add pythonImportsCheck
4a9dba6 
this not only gives us a "hint" of test coverage, but also proves the fix
from NixOS#97597 allows import of theano from within a sandboxed build
@risicle risicle mentioned this pull request Oct 18, 2020
Merged
10 tasks
risicle added a commit to risicle/nixpkgs that referenced this pull request Oct 18, 2020
@risicle
python3Packages.Theano: add pythonImportsCheck
f7b9e6a 
(modified cherry pick of commit 4a9dba6)

some tricks are required to make this work because NixOS#93560 and NixOS#97597
didn't make it into this branch, but this example shows what is required
to import theano from within a nix build environment on this branch.
jonringer pushed a commit that referenced this pull request Oct 19, 2020
@risicle @jonringer
python3Packages.Theano: add pythonImportsCheck
9b01e41 
(modified cherry pick of commit 4a9dba6)

some tricks are required to make this work because #93560 and #97597
didn't make it into this branch, but this example shows what is required
to import theano from within a nix build environment on this branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vcunat vcunat vcunat approved these changes

Assignees
No one assigned
Labels
2.status: merge conflict 10.rebuild-darwin: 0 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@twhitehead @vcunat