From 8132d0a12e1b1d30973ae2c543622a46c24ec075 Mon Sep 17 00:00:00 2001 From: Bruce Toll <4109762+tollb@users.noreply.github.com> Date: Sun, 17 Feb 2019 14:34:31 -0500 Subject: [PATCH 1/2] Fix nix-build --check -K in sandbox w/o root Temporarily add user-write permission to build directory so that it can be moved out of the sandbox to the store with a .check suffix. This is necessary because the build directory has already had its permissions set read-only, but write permission is required to update the directory's parent link to move it out of the sandbox. Updated the related --check "derivation may not be deterministic" messages to consistently use the real store paths. Added test for non-root sandbox nix-build --check -K to demonstrate issue and help prevent regressions. --- src/libstore/build.cc | 30 ++++++++++++++++++++++++++---- tests/linux-sandbox.sh | 7 +++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 760663ac9..b4207e1b8 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3537,6 +3537,29 @@ StorePathSet parseReferenceSpecifiers(Store & store, const BasicDerivation & drv } +static void moveCheckToStore(const Path & src, const Path & dst) +{ + /* For the rename of directory to succeed, we must be running as root or + the directory must be made temporarily writable (to update the + directory's parent link ".."). */ + struct stat st; + if (lstat(src.c_str(), &st) == -1) { + throw SysError(format("getting attributes of path '%1%'") % src); + } + + bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); + + if (changePerm) + chmod_(src, st.st_mode | S_IWUSR); + + if (rename(src.c_str(), dst.c_str())) + throw SysError(format("renaming '%1%' to '%2%'") % src % dst); + + if (changePerm) + chmod_(dst, st.st_mode); +} + + void DerivationGoal::registerOutputs() { /* When using a build hook, the build hook can register the output @@ -3715,8 +3738,7 @@ void DerivationGoal::registerOutputs() if (settings.runDiffHook || settings.keepFailed) { Path dst = worker.store.toRealPath(path + checkSuffix); deletePath(dst); - if (rename(actualPath.c_str(), dst.c_str())) - throw SysError(format("renaming '%1%' to '%2%'") % actualPath % dst); + moveCheckToStore(actualPath, dst); handleDiffHook( buildUser ? buildUser->getUID() : getuid(), @@ -3724,10 +3746,10 @@ void DerivationGoal::registerOutputs() path, dst, worker.store.printStorePath(drvPath), tmpDir); throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs from '%s'", - worker.store.printStorePath(drvPath), path, dst); + worker.store.printStorePath(drvPath), worker.store.toRealPath(path), dst); } else throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs", - worker.store.printStorePath(drvPath), path); + worker.store.printStorePath(drvPath), worker.store.toRealPath(path)); } /* Since we verified the build, it's now ultimately trusted. */ diff --git a/tests/linux-sandbox.sh b/tests/linux-sandbox.sh index 52967d07d..16abd974c 100644 --- a/tests/linux-sandbox.sh +++ b/tests/linux-sandbox.sh @@ -28,3 +28,10 @@ nix cat-store $outPath/foobar | grep FOOBAR # Test --check without hash rewriting. nix-build dependencies.nix --no-out-link --check --sandbox-paths /nix/store + +# Test that sandboxed builds with --check and -K can move .check directory to store +nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link + +(! nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link --check -K 2> $TEST_ROOT/log) +if grep -q 'error: renaming' $TEST_ROOT/log; then false; fi +grep -q 'may not be deterministic' $TEST_ROOT/log From e8bd1bc732ff19f33e6915d14e15df1d7de612c0 Mon Sep 17 00:00:00 2001 From: Bruce Toll <4109762+tollb@users.noreply.github.com> Date: Fri, 10 Apr 2020 18:20:12 -0400 Subject: [PATCH 2/2] Add test case for temporary directories on darwin A test case for correct handling of temporary directory deletion that was added to check.sh as part of PR #2689 was initially disabled for Darwin because of a directory permission issue in PR #2688. Now that the issue in PR #2688 is fixed, this commit enables the test case for Darwin. --- tests/check.sh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/check.sh b/tests/check.sh index b423dc0b5..5f25d04cb 100644 --- a/tests/check.sh +++ b/tests/check.sh @@ -49,13 +49,8 @@ checkBuildTempDirRemoved $TEST_ROOT/log nix-build check.nix -A nondeterministic --argstr checkBuildId $checkBuildId \ --no-out-link --check --keep-failed 2> $TEST_ROOT/log || status=$? - -# The above nix-build fails with status=1 on darwin (not sure why) -# ...but the primary purpose of the test case is to verify the temp directory is retained -if [ "$(uname -s)" != "Darwin" ]; then grep 'may not be deterministic' $TEST_ROOT/log [ "$status" = "104" ] -fi if checkBuildTempDirRemoved $TEST_ROOT/log; then false; fi clearStore