From 32effccb51783a36ce88607a7a404083e84ab3c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Mon, 11 Jul 2022 15:16:19 +0200 Subject: [PATCH 01/23] Add some tests for the CLI completion --- tests/completions.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++ tests/local.mk | 1 + 2 files changed, 49 insertions(+) create mode 100644 tests/completions.sh diff --git a/tests/completions.sh b/tests/completions.sh new file mode 100644 index 000000000..4d6beccf8 --- /dev/null +++ b/tests/completions.sh @@ -0,0 +1,48 @@ +source common.sh + +cd "$TEST_ROOT" + +mkdir -p dep && pushd dep +cat < flake.nix +{ + outputs = i: { }; +} +EOF +popd +mkdir -p foo && pushd foo +cat < flake.nix +{ + inputs.a.url = "path:$(realpath ../dep)"; + + outputs = i: { + sampleOutput = 1; + }; +} +EOF + +popd + +# Test the completion of a subcommand +[[ $(printf "normal\nbuild\t\n") == $(NIX_GET_COMPLETIONS=1 nix buil) ]] +[[ $(printf "normal\nmetadata\t\n") == $(NIX_GET_COMPLETIONS=2 nix flake metad) ]] + +# Filename completion +[[ $(printf "filenames\n./foo\t\n") == $(NIX_GET_COMPLETIONS=2 nix build ./f) ]] +[[ $(printf "filenames\n") == $(NIX_GET_COMPLETIONS=2 nix build ./nonexistent) ]] + +# Input override completion +[[ $(printf "normal\na\t\n") == $(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '') ]] + +# Cli flag completion +NIX_GET_COMPLETIONS=2 nix build --log-form | grep -- "--log-format" + +# Config option completion +## With `--option` +NIX_GET_COMPLETIONS=3 nix build --option allow-import-from | grep -- "allow-import-from-derivation" +## As a cli flag – not working atm +# NIX_GET_COMPLETIONS=2 nix build --allow-import-from | grep -- "allow-import-from-derivation" + + +# Attr path completions +[[ $(printf "attrs\n./foo#sampleOutput\t\n") == $(NIX_GET_COMPLETIONS=2 nix eval ./foo\#sam) ]] +[[ $(printf "attrs\noutputs\t\n") == $(NIX_GET_COMPLETIONS=4 nix eval --file ./foo/flake.nix outp) ]] diff --git a/tests/local.mk b/tests/local.mk index ae15c70f9..e0579f503 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -102,6 +102,7 @@ nix_tests = \ suggestions.sh \ store-ping.sh \ fetchClosure.sh \ + completion.sh \ impure-derivations.sh ifeq ($(HAVE_LIBCPUID), 1) From 260fb837de849a10571f19f81adbd6091a28c815 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 12 Jul 2022 09:19:05 +0200 Subject: [PATCH 02/23] Fix the name of the completions test --- tests/local.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/local.mk b/tests/local.mk index e0579f503..82e47729b 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -102,7 +102,7 @@ nix_tests = \ suggestions.sh \ store-ping.sh \ fetchClosure.sh \ - completion.sh \ + completions.sh \ impure-derivations.sh ifeq ($(HAVE_LIBCPUID), 1) From 07e14d3ef09e7852fce3a3850cd50eea032e3753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 12 Jul 2022 09:19:15 +0200 Subject: [PATCH 03/23] Harden the comparisons in the completion test - Don't use `printf` for the expected result, but just use bash's `$' '` litteral strings - Quote the `nix` call result - Invert the order in the comparisons (just because it feels more natural) --- tests/completions.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/completions.sh b/tests/completions.sh index 4d6beccf8..b7bb31aea 100644 --- a/tests/completions.sh +++ b/tests/completions.sh @@ -23,15 +23,15 @@ EOF popd # Test the completion of a subcommand -[[ $(printf "normal\nbuild\t\n") == $(NIX_GET_COMPLETIONS=1 nix buil) ]] -[[ $(printf "normal\nmetadata\t\n") == $(NIX_GET_COMPLETIONS=2 nix flake metad) ]] +[[ "$(NIX_GET_COMPLETIONS=1 nix buil)" == $'normal\nbuild\t' ]] +[[ "$(NIX_GET_COMPLETIONS=2 nix flake metad)" == $'normal\nmetadata\t' ]] # Filename completion -[[ $(printf "filenames\n./foo\t\n") == $(NIX_GET_COMPLETIONS=2 nix build ./f) ]] -[[ $(printf "filenames\n") == $(NIX_GET_COMPLETIONS=2 nix build ./nonexistent) ]] +[[ "$(NIX_GET_COMPLETIONS=2 nix build ./f)" == $'filenames\n./foo\t' ]] +[[ "$(NIX_GET_COMPLETIONS=2 nix build ./nonexistent)" == $'filenames' ]] # Input override completion -[[ $(printf "normal\na\t\n") == $(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '') ]] +[[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '')" == $'normal\na\t' ]] # Cli flag completion NIX_GET_COMPLETIONS=2 nix build --log-form | grep -- "--log-format" @@ -44,5 +44,5 @@ NIX_GET_COMPLETIONS=3 nix build --option allow-import-from | grep -- "allow-impo # Attr path completions -[[ $(printf "attrs\n./foo#sampleOutput\t\n") == $(NIX_GET_COMPLETIONS=2 nix eval ./foo\#sam) ]] -[[ $(printf "attrs\noutputs\t\n") == $(NIX_GET_COMPLETIONS=4 nix eval --file ./foo/flake.nix outp) ]] +[[ "$(NIX_GET_COMPLETIONS=2 nix eval ./foo\#sam)" == $'attrs\n./foo#sampleOutput\t' ]] +[[ "$(NIX_GET_COMPLETIONS=4 nix eval --file ./foo/flake.nix outp)" == $'attrs\noutputs\t' ]] From 21c443d4fd0dc4e28f4af085aef711d5ce30c5e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 12 Jul 2022 09:37:57 +0200 Subject: [PATCH 04/23] Test the tilde expansion for the flake completion Also add a disabled test for when the `--override-input` flag comes *before* the flake ref --- tests/completions.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/completions.sh b/tests/completions.sh index b7bb31aea..a510e28b0 100644 --- a/tests/completions.sh +++ b/tests/completions.sh @@ -32,6 +32,10 @@ popd # Input override completion [[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '')" == $'normal\na\t' ]] +## With tilde expansion +[[ "$(HOME=$PWD NIX_GET_COMPLETIONS=4 nix build '~/foo' --override-input '')" == $'normal\na\t' ]] +## Out of order – not working atm. Should have been fixed by #6693 but apparently not +# [[ "$(NIX_GET_COMPLETIONS=3 nix build --override-input '' ./foo)" == $'normal\na\t' ]] # Cli flag completion NIX_GET_COMPLETIONS=2 nix build --log-form | grep -- "--log-format" From d34a333e2ea6f7c3a8f86b821edd0542661d036a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Jul 2022 10:25:28 +0200 Subject: [PATCH 05/23] =?UTF-8?q?Fix=20the=20=E2=80=9Cout=20of=20order?= =?UTF-8?q?=E2=80=9D=20completion=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--override-input` id snarky because it takes two arguments, so it doesn't play well when completed in the middle of the CLI (since the argument just after gets interpreted as its second argument). So use `--update-input` instead --- tests/completions.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/completions.sh b/tests/completions.sh index a510e28b0..75b567146 100644 --- a/tests/completions.sh +++ b/tests/completions.sh @@ -34,8 +34,8 @@ popd [[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '')" == $'normal\na\t' ]] ## With tilde expansion [[ "$(HOME=$PWD NIX_GET_COMPLETIONS=4 nix build '~/foo' --override-input '')" == $'normal\na\t' ]] -## Out of order – not working atm. Should have been fixed by #6693 but apparently not -# [[ "$(NIX_GET_COMPLETIONS=3 nix build --override-input '' ./foo)" == $'normal\na\t' ]] +## Out of order +[[ "$(NIX_GET_COMPLETIONS=3 nix build --update-input '' ./foo)" == $'normal\na\t' ]] # Cli flag completion NIX_GET_COMPLETIONS=2 nix build --log-form | grep -- "--log-format" From b052e7e71ddf85921580415039a5a86a98ce042c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Jul 2022 10:29:47 +0200 Subject: [PATCH 06/23] Add some more completion tests - Test another command than `build` - Test with two input flakes --- tests/completions.sh | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/completions.sh b/tests/completions.sh index 75b567146..522aa1c86 100644 --- a/tests/completions.sh +++ b/tests/completions.sh @@ -2,25 +2,32 @@ source common.sh cd "$TEST_ROOT" -mkdir -p dep && pushd dep -cat < flake.nix +mkdir -p dep +cat < dep/flake.nix { outputs = i: { }; } EOF -popd -mkdir -p foo && pushd foo -cat < flake.nix +mkdir -p foo +cat < foo/flake.nix { - inputs.a.url = "path:$(realpath ../dep)"; + inputs.a.url = "path:$(realpath dep)"; outputs = i: { sampleOutput = 1; }; } EOF +mkdir -p bar +cat < bar/flake.nix +{ + inputs.b.url = "path:$(realpath dep)"; -popd + outputs = i: { + sampleOutput = 1; + }; +} +EOF # Test the completion of a subcommand [[ "$(NIX_GET_COMPLETIONS=1 nix buil)" == $'normal\nbuild\t' ]] @@ -32,10 +39,14 @@ popd # Input override completion [[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '')" == $'normal\na\t' ]] +[[ "$(NIX_GET_COMPLETIONS=5 nix flake show ./foo --override-input '')" == $'normal\na\t' ]] +## With multiple input flakes +[[ "$(NIX_GET_COMPLETIONS=5 nix build ./foo ./bar --override-input '')" == $'normal\na\t\nb\t' ]] ## With tilde expansion [[ "$(HOME=$PWD NIX_GET_COMPLETIONS=4 nix build '~/foo' --override-input '')" == $'normal\na\t' ]] ## Out of order [[ "$(NIX_GET_COMPLETIONS=3 nix build --update-input '' ./foo)" == $'normal\na\t' ]] +[[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --update-input '' ./bar)" == $'normal\na\t\nb\t' ]] # Cli flag completion NIX_GET_COMPLETIONS=2 nix build --log-form | grep -- "--log-format" @@ -46,7 +57,6 @@ NIX_GET_COMPLETIONS=3 nix build --option allow-import-from | grep -- "allow-impo ## As a cli flag – not working atm # NIX_GET_COMPLETIONS=2 nix build --allow-import-from | grep -- "allow-import-from-derivation" - # Attr path completions [[ "$(NIX_GET_COMPLETIONS=2 nix eval ./foo\#sam)" == $'attrs\n./foo#sampleOutput\t' ]] [[ "$(NIX_GET_COMPLETIONS=4 nix eval --file ./foo/flake.nix outp)" == $'attrs\noutputs\t' ]] From 2532fee157adf45bcd262f0f24a1740990df6e76 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Jul 2022 15:06:11 +0200 Subject: [PATCH 07/23] On test failures, print a bash stack trace This makes it easier to identify what command failed. It looks like: follow-paths.sh: test failed at: main in follow-paths.sh:54 --- tests/common.sh.in | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/common.sh.in b/tests/common.sh.in index 5efd025ee..79da10199 100644 --- a/tests/common.sh.in +++ b/tests/common.sh.in @@ -119,11 +119,11 @@ killDaemon() { } restartDaemon() { - [[ -z "${pidDaemon:-}" ]] && return 0 + [[ -z "${pidDaemon:-}" ]] && return 0 - killDaemon - unset NIX_REMOTE - startDaemon + killDaemon + unset NIX_REMOTE + startDaemon } if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then @@ -190,4 +190,15 @@ if [[ -n "${NIX_DAEMON_PACKAGE:-}" ]]; then startDaemon fi +onError() { + set +x + echo "$0: test failed at:" >&2 + for ((i = 1; i < 16; i++)); do + if [[ -z ${BASH_SOURCE[i]} ]]; then break; fi + echo " ${FUNCNAME[i]} in ${BASH_SOURCE[i]}:${BASH_LINENO[i-1]}" >&2 + done +} + +trap onError ERR + fi # COMMON_SH_SOURCED From ff49c75502845d9938f3a479f1696ee30d3b45b1 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Jul 2022 17:46:26 +0200 Subject: [PATCH 08/23] Disable auto-chroot if $NIX_STORE_DIR is set Fixes #6732. --- src/libstore/store-api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 05353bce2..45c53f23e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1321,7 +1321,7 @@ std::shared_ptr openFromNonUri(const std::string & uri, const Store::Para else if (pathExists(settings.nixDaemonSocketFile)) return std::make_shared(params); #if __linux__ - else if (!pathExists(stateDir) && params.empty() && getuid() != 0) { + else if (!pathExists(stateDir) && params.empty() && getuid() != 0 && !getEnv("NIX_STORE_DIR").has_value()) { /* If /nix doesn't exist, there is no daemon socket, and we're not root, then automatically set up a chroot store in ~/.local/share/nix/root. */ From 99208bb8ccd7387c09907e7b17d0091f2f767b14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Thu, 14 Jul 2022 17:36:31 -0500 Subject: [PATCH 09/23] curl: patch for netrc regression in Nix --- flake.lock | 6 +++--- flake.nix | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/flake.lock b/flake.lock index 01e4f506a..a66c9cb1b 100644 --- a/flake.lock +++ b/flake.lock @@ -18,11 +18,11 @@ }, "nixpkgs": { "locked": { - "lastModified": 1653988320, - "narHash": "sha256-ZaqFFsSDipZ6KVqriwM34T739+KLYJvNmCWzErjAg7c=", + "lastModified": 1657693803, + "narHash": "sha256-G++2CJ9u0E7NNTAi9n5G8TdDmGJXcIjkJ3NF8cetQB8=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "2fa57ed190fd6c7c746319444f34b5917666e5c1", + "rev": "365e1b3a859281cf11b94f87231adeabbdd878a2", "type": "github" }, "original": { diff --git a/flake.nix b/flake.nix index 1ee040441..1b26460e7 100644 --- a/flake.nix +++ b/flake.nix @@ -108,7 +108,7 @@ ++ lib.optionals stdenv.hostPlatform.isLinux [(buildPackages.util-linuxMinimal or buildPackages.utillinuxMinimal)]; buildDeps = - [ curl + [ (curl.override { patchNetrcRegression = true; }) bzip2 xz brotli editline openssl sqlite libarchive @@ -363,7 +363,7 @@ buildInputs = [ nix - curl + (curl.override { patchNetrcRegression = true; }) bzip2 xz pkgs.perl From 04386f7d69d9c370eb4367ca41d89ac5990ac02e Mon Sep 17 00:00:00 2001 From: Jeremy Fleischman Date: Thu, 14 Jul 2022 23:11:02 -0700 Subject: [PATCH 10/23] nix develop: do not assume that saved vars are set This fixes https://github.com/NixOS/nix/issues/6809 --- src/nix/develop.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 6d9ad9942..ba7ba7c25 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -288,8 +288,10 @@ struct Common : InstallableCommand, MixProfile out << "unset shellHook\n"; - for (auto & var : savedVars) + for (auto & var : savedVars) { + out << fmt("%s=${%s:-}\n", var, var); out << fmt("nix_saved_%s=\"$%s\"\n", var, var); + } buildEnvironment.toBash(out, ignoreVars); From 3bcd7a5474f065a6e94a60b3042a0d42ed0184ec Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 15 Jul 2022 12:32:29 +0200 Subject: [PATCH 11/23] Disable auto-chroot if $NIX_STATE_DIR is set Issue #6732. --- src/libstore/store-api.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 45c53f23e..91dbd991e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1321,7 +1321,12 @@ std::shared_ptr openFromNonUri(const std::string & uri, const Store::Para else if (pathExists(settings.nixDaemonSocketFile)) return std::make_shared(params); #if __linux__ - else if (!pathExists(stateDir) && params.empty() && getuid() != 0 && !getEnv("NIX_STORE_DIR").has_value()) { + else if (!pathExists(stateDir) + && params.empty() + && getuid() != 0 + && !getEnv("NIX_STORE_DIR").has_value() + && !getEnv("NIX_STATE_DIR").has_value()) + { /* If /nix doesn't exist, there is no daemon socket, and we're not root, then automatically set up a chroot store in ~/.local/share/nix/root. */ From b88fb50e218cd3099cbceace48f7cfdf50a8f11f Mon Sep 17 00:00:00 2001 From: Alex Wied Date: Wed, 6 Jul 2022 11:28:23 -0400 Subject: [PATCH 12/23] fix(libstore): allow Nix to access all Rosetta 2 paths on MacOS Fixes: #5884 --- src/libstore/sandbox-defaults.sb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/sandbox-defaults.sb b/src/libstore/sandbox-defaults.sb index 56b35c3fe..d9d710559 100644 --- a/src/libstore/sandbox-defaults.sb +++ b/src/libstore/sandbox-defaults.sb @@ -98,7 +98,9 @@ (allow file* (literal "/private/var/select/sh")) -; Allow Rosetta 2 to run x86_64 binaries on aarch64-darwin. +; Allow Rosetta 2 to run x86_64 binaries on aarch64-darwin (and vice versa). (allow file-read* (subpath "/Library/Apple/usr/libexec/oah") - (subpath "/System/Library/Apple/usr/libexec/oah")) + (subpath "/System/Library/Apple/usr/libexec/oah") + (subpath "/System/Library/LaunchDaemons/com.apple.oahd.plist") + (subpath "/Library/Apple/System/Library/LaunchDaemons/com.apple.oahd.plist")) From 8ea3a911aa81d41efdff231f4b42b11d8861a000 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 14:39:22 -0700 Subject: [PATCH 13/23] local-derivation-goal.cc: improve error messages when sandboxing fails The failure modes for nix's sandboxing setup are pretty complicated. When nix is unable to set up the sandbox, let's provide more detail about what went wrong. Specifically: * Make sure the error message includes the word "sandbox" so the user knows that the failure was related to sandboxing. * If `--option sandbox-fallback false` was provided, and removing it would have allowed further attempts to make progress, let the user know. --- src/libstore/build/local-derivation-goal.cc | 24 +++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index d1ec91ed5..86a59e427 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -850,13 +850,23 @@ void LocalDerivationGoal::startBuilder() flags &= ~CLONE_NEWUSER; child = clone(childEntry, stack + stackSize, flags, this); } - /* Otherwise exit with EPERM so we can handle this in the - parent. This is only done when sandbox-fallback is set - to true (the default). */ - if (child == -1 && (errno == EPERM || errno == EINVAL) && settings.sandboxFallback) - _exit(1); - if (child == -1) throw SysError("cloning builder process"); - + if (child == -1) + switch(errno) { + case EPERM: + case EINVAL: { + /* Otherwise exit with EPERM so we can handle this in the + parent. This is only done when sandbox-fallback is set + to true (the default). */ + if (settings.sandboxFallback) + _exit(1); + /* Mention sandbox-fallback in the error message so the user + knows that having it disabled contributed to the + unrecoverability of this failure */ + throw SysError("creating sandboxed builder process using clone(), without sandbox-fallback"); + } + default: + throw SysError("creating sandboxed builder process using clone()"); + } writeFull(builderOut.writeSide.get(), fmt("%d %d\n", usingUserNamespace, child)); _exit(0); From 90830b1074cd09b58adde859fb1741a33390412f Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 19:28:13 -0700 Subject: [PATCH 14/23] local-derivation-goal.cc: warn if failing due to max_user_namespaces==0 This commit uses `warn()` to notify the user if sandbox setup fails with errno==EPERM and /proc/sys/user/max_user_namespaces is missing or zero, since that is at least part of the reason why sandbox setup failed. Note that `echo -n 0 > /proc/sys/user/max_user_namespaces` or equivalent at boot time has been the recommended mitigation for several Linux LPE vulnerabilities over the past few years. Many users have applied this mitigation and then forgotten that they have done so. --- src/libstore/build/local-derivation-goal.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 86a59e427..674b2eaa3 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -859,6 +859,8 @@ void LocalDerivationGoal::startBuilder() to true (the default). */ if (settings.sandboxFallback) _exit(1); + if (!userNamespacesEnabled && errno==EPERM) + warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 8d35f387dcfa61c59f898de88ef45f3f97817267 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 19:37:27 -0700 Subject: [PATCH 15/23] local-derivation-goal.cc: warn if failing and /proc/self/ns/user missing This commit causes nix to `warn()` if sandbox setup has failed and `/proc/self/ns/user` does not exist. This is usually a sign that the kernel was compiled without `CONFIG_USER_NS=y`, which is required for sandboxing. --- src/libstore/build/local-derivation-goal.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 674b2eaa3..3aa85e264 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -861,6 +861,9 @@ void LocalDerivationGoal::startBuilder() _exit(1); if (!userNamespacesEnabled && errno==EPERM) warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + Path procSelfNsUser = "/proc/self/ns/user"; + if (!pathExists(procSelfNsUser)) + warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 6fc56318bf32f715de8634c199c0fb812f813a8c Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sun, 17 Jul 2022 01:23:27 -0700 Subject: [PATCH 16/23] local-derivation-goal.cc: add comment re: CLONE_NEWUSER local-derivation-goal.cc contains a comment stating that "Some distros patch Linux to not allow unprivileged user namespaces." Let's give a pointer to a common version of this patch for those who want more details about this failure mode. --- src/libstore/build/local-derivation-goal.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 3aa85e264..1c7618045 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -845,6 +845,7 @@ void LocalDerivationGoal::startBuilder() /* Some distros patch Linux to not allow unprivileged * user namespaces. If we get EPERM or EINVAL, try * without CLONE_NEWUSER and see if that works. + * Details: https://salsa.debian.org/kernel-team/linux/-/commit/d98e00eda6bea437e39b9e80444eee84a32438a6 */ usingUserNamespace = false; flags &= ~CLONE_NEWUSER; From c8c6203c2c6912a52c5f08881c453a04e7fc3f58 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sun, 17 Jul 2022 01:27:22 -0700 Subject: [PATCH 17/23] local-derivation-goal.cc: detect unprivileged_userns_clone failure mode The workaround for "Some distros patch Linux" mentioned in local-derivation-goal.cc will not help in the `--option sandbox-fallback false` case. To provide the user more helpful guidance on how to get the sandbox working, let's check to see if the `/proc` node created by the aforementioned patch is present and configured in a way that will cause us problems. If so, give the user a suggestion for how to troubleshoot the problem. --- src/libstore/build/local-derivation-goal.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 1c7618045..047c5c8ea 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -862,6 +862,13 @@ void LocalDerivationGoal::startBuilder() _exit(1); if (!userNamespacesEnabled && errno==EPERM) warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + if (userNamespacesEnabled) { + Path procSysKernelUnprivilegedUsernsClone = "/proc/sys/kernel/unprivileged_userns_clone"; + if (pathExists(procSysKernelUnprivilegedUsernsClone) + && trim(readFile(procSysKernelUnprivilegedUsernsClone)) == "0") { + warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); + } + } Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); From 5f51539f88227285866843f1383fd47d80fd5918 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:30:52 -0700 Subject: [PATCH 18/23] change warn() to notice() --- src/libstore/build/local-derivation-goal.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 047c5c8ea..595149f0a 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -861,17 +861,17 @@ void LocalDerivationGoal::startBuilder() if (settings.sandboxFallback) _exit(1); if (!userNamespacesEnabled && errno==EPERM) - warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { Path procSysKernelUnprivilegedUsernsClone = "/proc/sys/kernel/unprivileged_userns_clone"; if (pathExists(procSysKernelUnprivilegedUsernsClone) && trim(readFile(procSysKernelUnprivilegedUsernsClone)) == "0") { - warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); + notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); } } Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) - warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); + notice("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 99fcc91f67ece5a9646065665395f496d6a0cb84 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:33:12 -0700 Subject: [PATCH 19/23] as requested by @thufschmitt https://github.com/NixOS/nix/pull/6814#discussion_r924275777 --- src/libstore/build/local-derivation-goal.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 595149f0a..43df41e34 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -855,11 +855,6 @@ void LocalDerivationGoal::startBuilder() switch(errno) { case EPERM: case EINVAL: { - /* Otherwise exit with EPERM so we can handle this in the - parent. This is only done when sandbox-fallback is set - to true (the default). */ - if (settings.sandboxFallback) - _exit(1); if (!userNamespacesEnabled && errno==EPERM) notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { @@ -872,6 +867,11 @@ void LocalDerivationGoal::startBuilder() Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) notice("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); + /* Otherwise exit with EPERM so we can handle this in the + parent. This is only done when sandbox-fallback is set + to true (the default). */ + if (settings.sandboxFallback) + _exit(1); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From a9e75eca00f7efe361545c6e77ecb65244230dc3 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:49:33 -0700 Subject: [PATCH 20/23] error.hh: add additional constructor with explicit errno argument --- src/libutil/error.hh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libutil/error.hh b/src/libutil/error.hh index a53e9802e..3d1479c54 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -204,13 +204,19 @@ public: int errNo; template - SysError(const Args & ... args) + SysError(int errNo_, const Args & ... args) : Error("") { - errNo = errno; + errNo = errNo_; auto hf = hintfmt(args...); err.msg = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } + + template + SysError(const Args & ... args) + : SysError(errno, args ...) + { + } }; } From 36e1383b6b32abd414c8402dd66f7ef78f93f4e1 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:49:52 -0700 Subject: [PATCH 21/23] local-derivation-goal.cc: save global errno to the stack before performing tests which might clobber it --- src/libstore/build/local-derivation-goal.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 43df41e34..79a241ae0 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -851,10 +851,11 @@ void LocalDerivationGoal::startBuilder() flags &= ~CLONE_NEWUSER; child = clone(childEntry, stack + stackSize, flags, this); } - if (child == -1) + if (child == -1) { switch(errno) { case EPERM: case EINVAL: { + int errno_ = errno; if (!userNamespacesEnabled && errno==EPERM) notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { @@ -875,11 +876,12 @@ void LocalDerivationGoal::startBuilder() /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ - throw SysError("creating sandboxed builder process using clone(), without sandbox-fallback"); + throw SysError(errno_, "creating sandboxed builder process using clone(), without sandbox-fallback"); } default: throw SysError("creating sandboxed builder process using clone()"); } + } writeFull(builderOut.writeSide.get(), fmt("%d %d\n", usingUserNamespace, child)); _exit(0); From 1af5d798a4d10a07e995d420a759f2fe752b583a Mon Sep 17 00:00:00 2001 From: Alex Wied Date: Fri, 15 Jul 2022 00:14:29 -0400 Subject: [PATCH 22/23] libstore/globals.cc: Automatically set cores based on cgroup CPU limit By default, Nix sets the "cores" setting to the number of CPUs which are physically present on the machine. If cgroups are used to limit the CPU and memory consumption of a large Nix build, the OOM killer may be invoked. For example, consider a GitLab CI pipeline which builds a large software package. The GitLab runner spawns a container whose CPU is limited to 4 cores and whose memory is limited to 16 GiB. If the underlying machine has 64 cores, Nix will invoke the build with -j64. In many cases, that level of parallelism will invoke the OOM killer and the build will completely fail. This change sets the default value of "cores" to be ceil(cpu_quota / cpu_period), with a fallback to std::thread::hardware_concurrency() if cgroups v2 is not detected. --- src/libstore/globals.cc | 50 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 0f2ca4b15..48df839fa 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -11,6 +11,11 @@ #include #include +#if __linux__ +#include +#include +#endif + #include @@ -114,7 +119,50 @@ std::vector getUserConfigFiles() unsigned int Settings::getDefaultCores() { - return std::max(1U, std::thread::hardware_concurrency()); + unsigned int concurrency = std::max(1U, std::thread::hardware_concurrency()); + + #if __linux__ + FILE *fp = fopen("/proc/mounts", "r"); + if (!fp) + return concurrency; + + Strings cgPathParts; + + struct mntent *ent; + while ((ent = getmntent(fp))) { + std::string mountType, mountPath; + + mountType = ent->mnt_type; + mountPath = ent->mnt_dir; + + if (mountType == "cgroup2") { + cgPathParts.push_back(mountPath); + break; + } + } + + fclose(fp); + + if (cgPathParts.size() > 0 && pathExists("/proc/self/cgroup")) { + std::string currentCgroup = readFile("/proc/self/cgroup"); + Strings cgValues = tokenizeString(currentCgroup, ":"); + cgPathParts.push_back(trim(cgValues.back(), "\n")); + cgPathParts.push_back("cpu.max"); + std::string fullCgPath = canonPath(concatStringsSep("/", cgPathParts)); + + if (pathExists(fullCgPath)) { + std::string cpuMax = readFile(fullCgPath); + std::vector cpuMaxParts = tokenizeString>(cpuMax, " "); + std::string quota = cpuMaxParts[0]; + std::string period = trim(cpuMaxParts[1], "\n"); + + if (quota != "max") + concurrency = std::ceil(std::stoi(quota) / std::stof(period)); + } + } + #endif + + return concurrency; } StringSet Settings::getDefaultSystemFeatures() From 722de8ddcc875c7e8e9a228f9d88454bae31fd40 Mon Sep 17 00:00:00 2001 From: Alex Wied Date: Tue, 19 Jul 2022 02:09:46 -0400 Subject: [PATCH 23/23] libstore/globals.cc: Move cgroup detection to libutil --- src/libstore/globals.cc | 54 +++++------------------------------------ src/libutil/util.cc | 51 ++++++++++++++++++++++++++++++++++++++ src/libutil/util.hh | 3 +++ 3 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 48df839fa..d724897bb 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -11,11 +11,6 @@ #include #include -#if __linux__ -#include -#include -#endif - #include @@ -119,50 +114,13 @@ std::vector getUserConfigFiles() unsigned int Settings::getDefaultCores() { - unsigned int concurrency = std::max(1U, std::thread::hardware_concurrency()); + const unsigned int concurrency = std::max(1U, std::thread::hardware_concurrency()); + const unsigned int maxCPU = getMaxCPU(); - #if __linux__ - FILE *fp = fopen("/proc/mounts", "r"); - if (!fp) - return concurrency; - - Strings cgPathParts; - - struct mntent *ent; - while ((ent = getmntent(fp))) { - std::string mountType, mountPath; - - mountType = ent->mnt_type; - mountPath = ent->mnt_dir; - - if (mountType == "cgroup2") { - cgPathParts.push_back(mountPath); - break; - } - } - - fclose(fp); - - if (cgPathParts.size() > 0 && pathExists("/proc/self/cgroup")) { - std::string currentCgroup = readFile("/proc/self/cgroup"); - Strings cgValues = tokenizeString(currentCgroup, ":"); - cgPathParts.push_back(trim(cgValues.back(), "\n")); - cgPathParts.push_back("cpu.max"); - std::string fullCgPath = canonPath(concatStringsSep("/", cgPathParts)); - - if (pathExists(fullCgPath)) { - std::string cpuMax = readFile(fullCgPath); - std::vector cpuMaxParts = tokenizeString>(cpuMax, " "); - std::string quota = cpuMaxParts[0]; - std::string period = trim(cpuMaxParts[1], "\n"); - - if (quota != "max") - concurrency = std::ceil(std::stoi(quota) / std::stof(period)); - } - } - #endif - - return concurrency; + if (maxCPU > 0) + return maxCPU; + else + return concurrency; } StringSet Settings::getDefaultSystemFeatures() diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 28df30fef..be6fe091f 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -35,6 +35,9 @@ #ifdef __linux__ #include #include + +#include +#include #endif @@ -788,7 +791,55 @@ void drainFD(int fd, Sink & sink, bool block) } } +////////////////////////////////////////////////////////////////////// +unsigned int getMaxCPU() +{ + #if __linux__ + try { + FILE *fp = fopen("/proc/mounts", "r"); + if (!fp) + return 0; + + Strings cgPathParts; + + struct mntent *ent; + while ((ent = getmntent(fp))) { + std::string mountType, mountPath; + + mountType = ent->mnt_type; + mountPath = ent->mnt_dir; + + if (mountType == "cgroup2") { + cgPathParts.push_back(mountPath); + break; + } + } + + fclose(fp); + + if (cgPathParts.size() > 0 && pathExists("/proc/self/cgroup")) { + std::string currentCgroup = readFile("/proc/self/cgroup"); + Strings cgValues = tokenizeString(currentCgroup, ":"); + cgPathParts.push_back(trim(cgValues.back(), "\n")); + cgPathParts.push_back("cpu.max"); + std::string fullCgPath = canonPath(concatStringsSep("/", cgPathParts)); + + if (pathExists(fullCgPath)) { + std::string cpuMax = readFile(fullCgPath); + std::vector cpuMaxParts = tokenizeString>(cpuMax, " "); + std::string quota = cpuMaxParts[0]; + std::string period = trim(cpuMaxParts[1], "\n"); + + if (quota != "max") + return std::ceil(std::stoi(quota) / std::stof(period)); + } + } + } catch (Error &) { ignoreException(); } + #endif + + return 0; +} ////////////////////////////////////////////////////////////////////// diff --git a/src/libutil/util.hh b/src/libutil/util.hh index d3ed15b0b..29227ecc6 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -182,6 +182,9 @@ std::string drainFD(int fd, bool block = true, const size_t reserveSize=0); void drainFD(int fd, Sink & sink, bool block = true); +/* If cgroups are active, attempt to calculate the number of CPUs available. + If cgroups are unavailable or if cpu.max is set to "max", return 0. */ +unsigned int getMaxCPU(); /* Automatic cleanup of resources. */