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

getDefaultNixPath: actually respect {restrict,pure}-eval #7689

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

ncfavier
Copy link
Member

#4707 didn't do anything because getDefaultNixPath was called too early: at initialisation time, before CLI and config have been processed, when restrictEval and pureEval both have their default value false. Call it when initialising the EvalState instead, and use setDefault.

Add tests for the nix-path option and for --find-file . failing in restricted eval mode with no NIX_PATH.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

LGTM 👍, just some minor nitpicking, and some comments not directly related to this PR that can be addressed separately.

I've tested it by strace-ing the eval and confirms that Nix doesn't try to access these files anymore when it doesn't have to. Would be nice to have a test like that, but since we don't already depend on strace I don't know of an easy way to do it. So unless someone has a good simple idea we can skip this.

src/libexpr/eval.cc Outdated Show resolved Hide resolved
src/libexpr/eval.cc Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
Previously, getDefaultNixPath was called too early: at initialisation
time, before CLI and config have been processed, when `restrictEval` and
`pureEval` both have their default value `false`. Call it when
initialising the EvalState instead, and use `setDefault`.
@ncfavier ncfavier force-pushed the nix-path-restrict-eval branch from b60f41d to dba9173 Compare January 27, 2023 14:25
@ncfavier
Copy link
Member Author

Would be nice to have a test like that

Isn't this test enough? The idea is that we can ensure that the Nix path is empty using --find-file .. Currently this returns /home/user/.nix-defexpr/channels even with --restrict-eval and NIX_PATH unset.

@thufschmitt
Copy link
Member

Would be nice to have a test like that

Isn't this test enough? The idea is that we can ensure that the Nix path is empty using --find-file .. Currently this returns /home/user/.nix-defexpr/channels even with --restrict-eval and NIX_PATH unset.

It's not enough for #5884 in that it doesn't check that Nix doesn't try to access these paths at all. But that shouldn't prevent us from merging this.

@thufschmitt thufschmitt enabled auto-merge January 30, 2023 09:03
@thufschmitt thufschmitt merged commit d70b890 into NixOS:master Jan 30, 2023
@ncfavier ncfavier deleted the nix-path-restrict-eval branch January 30, 2023 10:20
edolstra added a commit that referenced this pull request Feb 28, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-45/26397/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants