From 0f7242ff8712939e64f049dc8e14663d2b3e3585 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 13 Jul 2023 13:17:17 -0400
Subject: [PATCH] Test nested sandboxing, and make nicer error

We were bedeviled by sandboxing issues when working on the layered
store. The problem ended up being that when we have nested nix builds,
and the inner store is inside the build dir (e.g. store is
`/build/nix-test/$name/store`, build dir is `/build`) bind mounts
clobber each other and store paths cannot be found.

After thoroughly cleaning up `local-derivation-goal.cc`, we might be
able to make that work. But that is a lot of work. For now, we just fail
earlier with a proper error message.

Finally, test this: nested sandboxing without the problematic store dir
should work, and with should fail with the expected error message.

Co-authored-by: Dylan Green <67574902+cidkidnix@users.noreply.github.com>
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
---
 src/libstore/build/local-derivation-goal.cc |  4 +++
 tests/local.mk                              |  3 ++-
 tests/nested-sandboxing.sh                  | 11 ++++++++
 tests/nested-sandboxing/command.sh          | 29 +++++++++++++++++++++
 tests/nested-sandboxing/runner.nix          | 24 +++++++++++++++++
 5 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 tests/nested-sandboxing.sh
 create mode 100644 tests/nested-sandboxing/command.sh
 create mode 100644 tests/nested-sandboxing/runner.nix

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index ee66ee500..e22a522a2 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -594,6 +594,10 @@ void LocalDerivationGoal::startBuilder()
             else
                 dirsInChroot[i.substr(0, p)] = {i.substr(p + 1), optional};
         }
+        if (hasPrefix(worker.store.storeDir, tmpDirInSandbox))
+        {
+            throw Error("`sandbox-build-dir` must not contain the storeDir");
+        }
         dirsInChroot[tmpDirInSandbox] = tmpDir;
 
         /* Add the closure of store paths to the chroot. */
diff --git a/tests/local.mk b/tests/local.mk
index bb80c5317..173bc84b3 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -138,7 +138,8 @@ nix_tests = \
   path-from-hash-part.sh \
   test-libstoreconsumer.sh \
   toString-path.sh \
-  read-only-store.sh
+  read-only-store.sh \
+  nested-sandboxing.sh
 
 ifeq ($(HAVE_LIBCPUID), 1)
 	nix_tests += compute-levels.sh
diff --git a/tests/nested-sandboxing.sh b/tests/nested-sandboxing.sh
new file mode 100644
index 000000000..d9fa788aa
--- /dev/null
+++ b/tests/nested-sandboxing.sh
@@ -0,0 +1,11 @@
+source common.sh
+# This test is run by `tests/nested-sandboxing/runner.nix` in an extra layer of sandboxing.
+[[ -d /nix/store ]] || skipTest "running this test without Nix's deps being drawn from /nix/store is not yet supported"
+
+requireSandboxSupport
+
+source ./nested-sandboxing/command.sh
+
+expectStderr 100 runNixBuild badStoreUrl 2 | grepQuiet '`sandbox-build-dir` must not contain'
+
+runNixBuild goodStoreUrl 5
diff --git a/tests/nested-sandboxing/command.sh b/tests/nested-sandboxing/command.sh
new file mode 100644
index 000000000..69366486c
--- /dev/null
+++ b/tests/nested-sandboxing/command.sh
@@ -0,0 +1,29 @@
+export NIX_BIN_DIR=$(dirname $(type -p nix))
+# TODO Get Nix and its closure more flexibly
+export EXTRA_SANDBOX="/nix/store $(dirname $NIX_BIN_DIR)"
+
+badStoreUrl () {
+    local altitude=$1
+    echo $TEST_ROOT/store-$altitude
+}
+
+goodStoreUrl () {
+    local altitude=$1
+    echo $("badStoreUrl" "$altitude")?store=/foo-$altitude
+}
+
+# The non-standard sandbox-build-dir helps ensure that we get the same behavior
+# whether this test is being run in a derivation as part of the nix build or
+# being manually run by a developer outside a derivation
+runNixBuild () {
+    local storeFun=$1
+    local altitude=$2
+    nix-build \
+        --no-substitute --no-out-link \
+        --store "$("$storeFun" "$altitude")" \
+        --extra-sandbox-paths "$EXTRA_SANDBOX" \
+        ./nested-sandboxing/runner.nix \
+        --arg altitude "$((altitude - 1))" \
+        --argstr storeFun "$storeFun" \
+        --sandbox-build-dir /build-non-standard
+}
diff --git a/tests/nested-sandboxing/runner.nix b/tests/nested-sandboxing/runner.nix
new file mode 100644
index 000000000..9a5822c88
--- /dev/null
+++ b/tests/nested-sandboxing/runner.nix
@@ -0,0 +1,24 @@
+{ altitude, storeFun }:
+
+with import ../config.nix;
+
+mkDerivation {
+  name = "nested-sandboxing";
+  busybox = builtins.getEnv "busybox";
+  EXTRA_SANDBOX = builtins.getEnv "EXTRA_SANDBOX";
+  buildCommand = if altitude == 0 then ''
+    echo Deep enough! > $out
+  '' else ''
+    cp -r ${../common} ./common
+    cp ${../common.sh} ./common.sh
+    cp ${../config.nix} ./config.nix
+    cp -r ${./.} ./nested-sandboxing
+
+    export PATH=${builtins.getEnv "NIX_BIN_DIR"}:$PATH
+
+    source common.sh
+    source ./nested-sandboxing/command.sh
+
+    runNixBuild ${storeFun} ${toString altitude} >> $out
+  '';
+}