Purity checking should accept $TMP and not just /tmp

[twhitehead]

Motivation for this change

The badPath routine used by the various compiler paths to check purity is hard coded to accept /tmp. This can cause issues with nix-shell as it overrides /tmp by setting the various temporary variables (TMP,TEMP,TMPDIR, and TEMPDIR) to /run/user/$UID.

Normally this is masked by the fact NIX_BUILD_TOP is also set to /run/user/$UID so the usage of temporary storage by the tools gets covered under NIX_BUILD_TOP. In circumstances when the TMP and NIX_BUILD_TOP don't coincide though, things fall apart.

Here is a small example that demonstrates this

$ nix-shell -A hello
$ export NIX_ENFORCE_PURITY=1
$ echo $NIX_BUILD_TOP
/run/user/1000
$ echo $TMP
/run/user/1000
$ export NIX_BUILD_TOP=/tmp
$ cd $NIX_BUILD_TOP
$ eval "${unpackPhase:-unpackPhase}"
$ cd hello-2.10
$ eval "${patchPhase:-patchPhase}"
$ eval "${configurePhase:-configurePhase}"
...
configure: error: in `/tmp/hello-2.10':
configure: error: C compiler cannot create executables
See `config.log' for more details

Another example is issue #91431.

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.

[FRidh]

@Ericson2314 @matthewbauer could any of you review this?

[jtojnar]

This looks alright but feels harder to reason about. Compare:

• beginning of p is not x
• p with x removed from the start (if there) is still p

Also had to consult https://guide.bash.academy/expansions/?=Parameter_Expansion#h2.2.2 for the latter.

[twhitehead]

I tried quite a few things and spent some time in the man page until I finally managed to come up with that. The really hard part was trying to come up with something that would let you default TMP to /tmp. 😁

Essentially we use the strip leading prefix of p expansion (${p#<prefix to try and strip>}} with each of our path prefixes we want the check

• if it manages to strip a leading prefix (string is altered) then we had a leading prefix match and the path is okay
• if it doesn't manage to strip a leading prefix (string isn't altered) then we didn't have a leading prefix match and the path is bad

This is quite flexible as the <prefix to try and strip> is itself expanded, so get to use all the power of expansion again when making our prefix to try and strip (in this case, the critical one being we can default TMP).

[vcunat]

This has some unpleasant consequences (in addition to breaking Darwin stdenv). In usual sandboxed builds we have TMP=/build, i.e. this check started firing by this PR. Various configure-like scripts want to detect stuff by compiling temporary files and they started now getting errors like impure path '/build/foo' used in link. Previously we "only" had checks that /build is not referenced in the final output, which really fit the use cases much better.

So far I don't know how frequent the Linux regressions are; most of them should be among this list; I just worked around the libbpf regression and I glimpsed e.g. dmd. @worldofpeace: I'd probably prefer not to have this PR at all in 20.09 (which is about to start stabilization now), as it will be adding even more stuff to fix before the approaching release (and ATM I'm not yet really convinced that the PR is a good idea).

[pbogdan]

Just to add to what @vcunat said WRT. Darwin failure in case it helps (and before I forget). The failure was happening during cmake build, I added some simple debugging to the cc-wrapper which produced the following:

SKIPPING BAD PATH -I/tmp/nix-build-cmake-boot-3.18.2.drv-0/cmake-3.18.2/Utilities/cmzlib
NIX_BUILD_TOP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0
TMP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0
SKIPPING BAD PATH -I/tmp/nix-build-cmake-boot-3.18.2.drv-0/cmake-3.18.2/Utilities/cmzlib/..
NIX_BUILD_TOP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0                                                                                      
TMP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0                                                                                                
SKIPPING BAD PATH -I/tmp/nix-build-cmake-boot-3.18.2.drv-0/cmake-3.18.2/Source
NIX_BUILD_TOP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0
TMP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0
SKIPPING BAD PATH -I/tmp/nix-build-cmake-boot-3.18.2.drv-0/cmake-3.18.2/Source/kwsys
NIX_BUILD_TOP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0
TMP: /private/tmp/nix-build-cmake-boot-3.18.2.drv-0

As I understand it $TMP (and other temp-related variables) are set to /private/tmp whereas cmake somehow generates the -I flags pointing to /tmp. However on Darwin the latter is simply a symlink to /private/tmp so the string (prefix?) comparison rejects them even though technically they should be allowed, and cmake build can't find some of its headers.

(As a side note it would perhaps be nice if there was some sort of indication a path was filtered out in this way).

[worldofpeace]

This has some unpleasant consequences (in addition to breaking Darwin stdenv). In usual sandboxed builds we have TMP=/build, i.e. this check started firing by this PR. Various configure-like scripts want to detect stuff by compiling temporary files and they started now getting errors like impure path '/build/foo' used in link. Previously we "only" had checks that /build is not referenced in the final output, which really fit the use cases much better.

So far I don't know how frequent the Linux regressions are; most of them should be among this list; I just worked around the libbpf regression and I glimpsed e.g. dmd. @worldofpeace: I'd probably prefer not to have this PR at all in 20.09 (which is about to start stabilization now), as it will be adding even more stuff to fix before the approaching release (and ATM I'm not yet really convinced that the PR is a good idea).

Sounds correct to me. Has this been already reverted?

[twhitehead]

If I'm understanding correctly, the issue is that /tmp is still being used in many places despite $TMP being set to a non-/tmp path. As another example from the hydra list

https://hydra.nixos.org/build/126586507/nixlog/1

Compiling ncftpget:                                                   [ERROR]  
  gcc -D_REENTRANT -O2 -W -Wall -Wno-format-y2k -fstack-protector-all -D_FORTIF
  Y_SOURCE=2 -DLINUX=59116 -DLINUX_GLIBC=23100 -Dsh_util -DO_S="linux-x86_64-gl
  ibc2.31" -DSYSCONFDIR="/nix/store/aaakh5iza6n1b0p246gi6w5mkx6y561b-ncftp-3.2.
  6/etc" -DHAVE_CONFIG_H -DLINUX=59116 -DLINUX_GLIBC=23100 -I/build/ncftp-3.2.6
   -I../libncftp -I../Strn -I../sio -I/build/ncftp-3.2.6 -I/build/ncftp-3.2.6/l
  ibncftp -I/build/ncftp-3.2.6/sio -I/build/ncftp-3.2.6/Strn gpshare.o bookmark
  .o preffw.o spoolutil.o util.o gl_getline.o version.o ncftpget.c -o ../bin/nc
  ftpget -L../libncftp -L../Strn -L../sio -L/build/ncftp-3.2.6/libncftp -L/buil
  d/ncftp-3.2.6/sio -L/build/ncftp-3.2.6/Strn -lncftp -lStrn -lsio -lresolv
  impure path `/tmp/ccAJpfhO.o' used in link

Perhaps a better patch then would have not been to replace /tmp with $TMP, but rather to accept both both /tmp and $TMP (and presumable $TEMP and $TMPDIR too, just to cover them all)? This would be easy to do.

[vcunat]

No, in the Linux cases I saw, $TMP was used correctly and linking stuff inside $TMP/ got newly refused (in the "usual" case of TMP=/build).

[vcunat]

@worldofpeace: looking at the PR-pings above, on master it's "reverted for Darwin", as a quick-fix measure, and on staging the revert is finished (e560459). Touching the file causes rebuild of everything...

[FRidh]

and on staging the revert is finished (e560459)

as well as 5ceea57.

[twhitehead]

Hummm. That is very odd as the purpose of the patch was to honour $TMP and not to ignore it. If you look at the original code

    # Otherwise, the path should refer to the store or some temporary
    # directory (including the build directory).
    test \
        "$p" != "/dev/null" -a \
        "${p:0:${#NIX_STORE}}" != "$NIX_STORE" -a \
        "${p:0:4}" != "/tmp" -a \
        "${p:0:${#NIX_BUILD_TOP}}" != "$NIX_BUILD_TOP"

it clearly entirely ignores the setting of $TMP and just checks for /tmp. The only reason it wasn't blowing things up in most cases was that $NIX_BUILD_TOP happened to also be set the same as $TMP.

@vcunat do you have an example package where the problem was that the patch wasn't honouring $TMP that I can dig into and see if I can figure out what went wrong? Thanks!

[vcunat]

@twhitehead: libbpf is quite a small package. You can test e.g. on parent of commit 2c145f4. Here is the compiler command on file in $TMP that gets failed with that "impure path" message.

[twhitehead]

Wouldn't the failure in this case be due to the earlier line that sets tfile assuming /tmp is the temporary location and not $TMP?

tfile=$(mktemp /tmp/test_reallocarray_XXXXXXXX.c)
...
"$@" $tfile -o $ofile >/dev/null 2>&1

Perhaps we are saying the same thing but in different ways? In any event, I believe this would be addressed by adjusting the patch to accept all of /tmp, $TMP, $TEMP, and $TMPDIR.

[vcunat]

Oh right... I didn't realize; that script is not using temporary dirs properly. They should use mktemp test_reallocarray_XXXXXXXX.c. EDIT: tested and filed upstream, regardless of how we resolve this PR generally.