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

nix develop: use proper permissions for $TMPDIR #6974

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

winterqt
Copy link
Member

Before this change, unsandboxed builds that tried to use passAsFile would fail when run within a devshell.

@@ -298,7 +298,7 @@ struct Common : InstallableCommand, MixProfile
for (auto & var : savedVars)
out << fmt("%s=\"$%s:$nix_saved_%s\"\n", var, var, var);

out << "export NIX_BUILD_TOP=\"$(mktemp -d -t nix-shell.XXXXXX)\"\n";
out << "export NIX_BUILD_TOP=\"$(mktemp -d -t nix-shell.XXXXXX)\"\nchmod 777 \"$NIX_BUILD_TOP\"\n";
Copy link
Member

Choose a reason for hiding this comment

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

You are really sure about 777? I would expect something more like 755.

Copy link
Member Author

Choose a reason for hiding this comment

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

/tmp on Linux is usually 777, that's what I went off of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the common perms for /tmp 1777 (note the sticky bit)? (At least it is on my systems.)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I thought the sticky bit signified that it was a directory for whatever reason. Fixed.

Before this change, unsandboxed builds that tried to use `passAsFile` would fail when run within a devshell.
@edolstra
Copy link
Member

I don't understand how this is supposed to fix passAsFile. The problem is that the environment variables recorded in the getDerivationEnvironment() refer to the sandbox, which doesn't exist anymore when we run the shell, e.g.

# nix develop
# echo $fooPath 
/build/.attr-1bp7cri8hplaz6hbz0v4f0nl44rl84q1sg25kgwqzipzd1mv89ic

Creating a world-writable directory definitely seems like something to be avoided.

Probably makeRcScript() should just process the passAsFile attributes from the original derivation and write them somewhere. The problem is that it's not entirely obvious where it should write them. E.g. it could write them to a temporary file that gets deleted when nix develop exits, but that wouldn't work for nix print-dev-env, and it might be annoying if the passAsFile paths change on every invocation. Or maybe it should just copy them to the Nix store...

@winterqt
Copy link
Member Author

winterqt commented Aug 31, 2022

I don't understand how this is supposed to fix passAsFile.

This fixes passAsFile from (unsandboxed) builds run within a devshell, i.e.

$ nix develop ...
$ nix build ...

Creating a world-writable directory definitely seems like something to be avoided.

So should the permissions be set to (1)755 to fix this issue?

I used 1777 since that's what most things expect $TMPDIR to be, though in this case 755 would probably suffice.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 3, 2023

Triaged in the Nix team meeting:

We couldn't figure out what problem this is solving exactly. @winterqt, we will pick this up again if you add a test that reproduces the bug. Postponed for now.

Complete discussion
  • @edolstra: when you use pass-as-file it doesn't work in nix develop because those env vars will refer to files in the sandbox in useDerviationEnvironment which don't exist any more
    • didn't understand what the change is supposed to do though
    • especially how the restricted permissions break the build
  • @Ericson2314: maybe the author should write a test and then we figure out what to do about it
  • @thufschmitt: why do we set NIX_BUILD_TOP anyway?
    • @edolstra: to simulate a regular build as closely as possibler
  • @edolstra: it's not even clear what exactly this fixes
  • postponed, waiting for author to provide a reproducer

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1

@fricklerhandwerk fricklerhandwerk marked this pull request as draft April 17, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants