From a2f0ba6a6dbb79efbb83ebe92b287f79b3f3af91 Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Mon, 16 Oct 2023 16:51:49 -0400 Subject: [PATCH 1/9] Fix transitive input locking. Fixes reproducibility issue described in #9143 Fixes #9143 --- src/libexpr/flake/flake.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index a6212c12f..5d9d60655 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -351,10 +351,13 @@ LockedFlake lockFlake( debug("old lock file: %s", oldLockFile); std::map overrides; + std::set explicitCliOverrides; std::set overridesUsed, updatesUsed; - for (auto & i : lockFlags.inputOverrides) + for (auto & i : lockFlags.inputOverrides) { overrides.insert_or_assign(i.first, FlakeInput { .ref = i.second }); + explicitCliOverrides.insert(i.first); + } LockFile newLockFile; @@ -425,6 +428,7 @@ LockedFlake lockFlake( ancestors? */ auto i = overrides.find(inputPath); bool hasOverride = i != overrides.end(); + bool hasCliOverride = explicitCliOverrides.find(inputPath) != explicitCliOverrides.end(); if (hasOverride) { overridesUsed.insert(inputPath); // Respect the “flakeness” of the input even if we @@ -460,7 +464,7 @@ LockedFlake lockFlake( if (oldLock && oldLock->originalRef == *input.ref - && !hasOverride) + && !hasCliOverride) { debug("keeping existing input '%s'", inputPathS); @@ -541,7 +545,7 @@ LockedFlake lockFlake( nuked the next time we update the lock file. That is, overrides are sticky unless you use --no-write-lock-file. */ - auto ref = input2.ref ? *input2.ref : *input.ref; + auto ref = (input2.ref && explicitCliOverrides.contains(inputPath)) ? *input2.ref : *input.ref; if (input.isFlake) { Path localPath = parentPath; From 311e2ad024441950cb1300e56c9745259deebdda Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Wed, 18 Oct 2023 10:37:06 -0400 Subject: [PATCH 2/9] Address review comments --- src/libexpr/flake/flake.cc | 2 +- tests/functional/flakes/follow-paths.sh | 76 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 5d9d60655..2c7e12ec9 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -428,7 +428,7 @@ LockedFlake lockFlake( ancestors? */ auto i = overrides.find(inputPath); bool hasOverride = i != overrides.end(); - bool hasCliOverride = explicitCliOverrides.find(inputPath) != explicitCliOverrides.end(); + bool hasCliOverride = explicitCliOverrides.contains(inputPath); if (hasOverride) { overridesUsed.insert(inputPath); // Respect the “flakeness” of the input even if we diff --git a/tests/functional/flakes/follow-paths.sh b/tests/functional/flakes/follow-paths.sh index 8573b5511..7f4e8bf5d 100644 --- a/tests/functional/flakes/follow-paths.sh +++ b/tests/functional/flakes/follow-paths.sh @@ -260,3 +260,79 @@ EOF checkRes=$(nix flake lock "$flakeFollowCycle" 2>&1 && fail "nix flake lock should have failed." || true) echo $checkRes | grep -F "error: follow cycle detected: [baz -> foo -> bar -> baz]" + + +# Test transitive input url locking +# This tests the following lockfile issue: https://github.com/NixOS/nix/issues/9143 +# +# We construct the following graph, where p->q means p has input q. +# +# A -> B -> C +# +# And override B/C to flake D, first in A's flake.nix and then with --override-input. +# +# A -> B -> D +flakeFollowsCustomUrlA="$TEST_ROOT/follows/custom-url/flakeA" +flakeFollowsCustomUrlB="$TEST_ROOT/follows/custom-url/flakeA/flakeB" +flakeFollowsCustomUrlC="$TEST_ROOT/follows/custom-url/flakeA/flakeB/flakeC" +flakeFollowsCustomUrlD="$TEST_ROOT/follows/custom-url/flakeA/flakeB/flakeD" + + +createGitRepo "$flakeFollowsCustomUrlA" +mkdir -p "$flakeFollowsCustomUrlB" +mkdir -p "$flakeFollowsCustomUrlC" +mkdir -p "$flakeFollowsCustomUrlD" + +cat > "$flakeFollowsCustomUrlD/flake.nix" < "$flakeFollowsCustomUrlC/flake.nix" < "$flakeFollowsCustomUrlB/flake.nix" < "$flakeFollowsCustomUrlA/flake.nix" < Date: Mon, 20 Nov 2023 03:37:02 -0700 Subject: [PATCH 3/9] flakes: check for flake.nix before complaining that lstat on it fails getFlake currently calls lstat (via isLink via canonPath) before it performs the sanity check that a flake.nix exists in the first place. This commit moves the check to before path canonicalization, so that failed symlink check operations don't throw before the check does. --- src/libexpr/flake/flake.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 54de53e0b..b128de31e 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -212,8 +212,16 @@ static Flake getFlake( auto [storePath, resolvedRef, lockedRef] = fetchOrSubstituteTree( state, originalRef, allowLookup, flakeCache); + // We need to guard against symlink attacks, but before we start doing + // filesystem operations we should make sure there's a flake.nix in the + // first place. + auto unsafeFlakeDir = state.store->toRealPath(storePath) + "/" + lockedRef.subdir; + auto unsafeFlakeFile = unsafeFlakeDir + "/flake.nix"; + if (!pathExists(unsafeFlakeFile)) + throw Error("source tree referenced by '%s' does not contain a '%s/flake.nix' file", lockedRef, lockedRef.subdir); + // Guard against symlink attacks. - auto flakeDir = canonPath(state.store->toRealPath(storePath) + "/" + lockedRef.subdir, true); + auto flakeDir = canonPath(unsafeFlakeDir, true); auto flakeFile = canonPath(flakeDir + "/flake.nix", true); if (!isInDir(flakeFile, state.store->toRealPath(storePath))) throw Error("'flake.nix' file of flake '%s' escapes from '%s'", @@ -226,9 +234,6 @@ static Flake getFlake( .storePath = storePath, }; - if (!pathExists(flakeFile)) - throw Error("source tree referenced by '%s' does not contain a '%s/flake.nix' file", lockedRef, lockedRef.subdir); - Value vInfo; state.evalFile(state.rootPath(CanonPath(flakeFile)), vInfo, true); // FIXME: symlink attack From 2ce8c9650b3e714f28d8685e48996141cba2df2c Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Thu, 23 Nov 2023 22:02:20 +0100 Subject: [PATCH 4/9] doc: primops: add more info for foldl (#9254) * doc: primops: add more info for foldl From the existing doc it is not obvious whether the first or the second argument is the accumulator. This is however relevant to know, as for certain scenarios, this might change the behavior. Co-authored-by: Valentin Gagarin --- src/libexpr/primops.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a8d44d8b7..7c0561413 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3179,9 +3179,16 @@ static RegisterPrimOp primop_foldlStrict({ .doc = R"( Reduce a list by applying a binary operator, from left to right, e.g. `foldl' op nul [x0 x1 x2 ...] = op (op (op nul x0) x1) x2) - ...`. For example, `foldl' (x: y: x + y) 0 [1 2 3]` evaluates to 6. - The return value of each application of `op` is evaluated immediately, - even for intermediate values. + ...`. + + For example, `foldl' (acc: elem: acc + elem) 0 [1 2 3]` evaluates + to `6` and `foldl' (acc: elem: { "${elem}" = elem; } // acc) {} + ["a" "b"]` evaluates to `{ a = "a"; b = "b"; }`. + + The first argument of `op` is the accumulator wheres the second + argument is the current element being processed. The return value + of each application of `op` is evaluated immediately, even for + intermediate values. )", .fun = prim_foldlStrict, }); From 5be0e6b314c216b0b51499fc488ca08272297469 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Fri, 24 Nov 2023 10:50:01 +0100 Subject: [PATCH 5/9] doc: primops: fix typo --- src/libexpr/primops.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7c0561413..ba735b435 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3185,7 +3185,7 @@ static RegisterPrimOp primop_foldlStrict({ to `6` and `foldl' (acc: elem: { "${elem}" = elem; } // acc) {} ["a" "b"]` evaluates to `{ a = "a"; b = "b"; }`. - The first argument of `op` is the accumulator wheres the second + The first argument of `op` is the accumulator whereas the second argument is the current element being processed. The return value of each application of `op` is evaluated immediately, even for intermediate values. From 6a94755b1240be654cadb463a9f528eeccf3787c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 24 Nov 2023 11:45:37 +0100 Subject: [PATCH 6/9] Allow user input in `git commit` We occasionnally commit to git repositories (like with `nix flake update --commit-lock-file`). This shells out to `git commit`, which might wait for user input (for a signing key passphrase for instance). Disable the progress bar while this is running to make sure that the user can enter it. --- src/libfetchers/git.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 2c5b70f53..8cd74057c 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -11,6 +11,8 @@ #include "fs-input-accessor.hh" #include "mounted-input-accessor.hh" #include "git-utils.hh" +#include "logging.hh" +#include "finally.hh" #include "fetch-settings.hh" @@ -314,6 +316,9 @@ struct GitInputScheme : InputScheme runProgram("git", true, { "-C", repoInfo.url, "--git-dir", repoInfo.gitDir, "add", "--intent-to-add", "--", std::string(path.rel()) }); + // Pause the logger to allow for user input (such as a gpg passphrase) in `git commit` + logger->pause(); + Finally restoreLogger([]() { logger->resume(); }); if (commitMsg) runProgram("git", true, { "-C", repoInfo.url, "--git-dir", repoInfo.gitDir, "commit", std::string(path.rel()), "-m", *commitMsg }); From 9aa63f70d7d861ba74764188410c0add730cd48d Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Fri, 24 Nov 2023 15:32:02 +0100 Subject: [PATCH 7/9] fricklerhandwerk: subscribe to documentation changes (#9422) * fricklerhandwerk: subscribe to documentation changes --- .github/CODEOWNERS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index ab5908649..39d595199 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -14,5 +14,12 @@ /doc @fricklerhandwerk *.md @fricklerhandwerk +# Documentation of built-in functions +src/libexpr/primops.cc @fricklerhandwerk @roberth +# Documentation on experimental features +src/libutil/experimental-features.cc @fricklerhandwerk +# Documentation on configuration settings +src/libstore/globals.hh @fricklerhandwerk + # Libstore layer /src/libstore @thufschmitt From 54b684765519dee1e74dca71605f8e7f6c8b0a25 Mon Sep 17 00:00:00 2001 From: ivan770 Date: Fri, 24 Nov 2023 11:17:35 -0500 Subject: [PATCH 8/9] doc: fix machine-specific capabilities leaking --- src/libstore/globals.hh | 14 +++++++++++--- src/libstore/store-api.hh | 5 ++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 27caf42c4..838d2aba2 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -183,7 +183,9 @@ public: command line switch and defaults to `1`. The value `0` means that the builder should use all available CPU cores in the system. )", - {"build-cores"}, false}; + {"build-cores"}, + // Don't document the machine-specific default value + false}; /** * Read-only mode. Don't copy stuff to the store, don't change @@ -699,7 +701,10 @@ public: Build systems will usually detect the target platform to be the current physical system and therefore produce machine code incompatible with what may be intended in the derivation. You should design your derivation's `builder` accordingly and cross-check the results when using this option against natively-built versions of your derivation. - )", {}, false}; + )", + {}, + // Don't document the machine-specific default value + false}; Setting systemFeatures{ this, @@ -744,7 +749,10 @@ public: [nspawn]: https://github.com/NixOS/nix/blob/67bcb99700a0da1395fa063d7c6586740b304598/tests/systemd-nspawn.nix. Included by default on Linux if the [`auto-allocate-uids`](#conf-auto-allocate-uids) setting is enabled. - )", {}, false}; + )", + {}, + // Don't document the machine-specific default value + false}; Setting substituters{ this, diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 32ad2aa44..8b6bf9aed 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -167,7 +167,10 @@ struct StoreConfig : public Config Optional [system features](@docroot@/command-ref/conf-file.md#conf-system-features) available on the system this store uses to build derivations. Example: `"kvm"` - )" }; + )", + {}, + // Don't document the machine-specific default value + false}; }; class Store : public std::enable_shared_from_this, public virtual StoreConfig From 213594721ace2b2e2dcf3d57615178a2c1690aa4 Mon Sep 17 00:00:00 2001 From: Peter Kolloch Date: Sat, 25 Nov 2023 17:21:49 +0100 Subject: [PATCH 9/9] gitignore: Also ignore .DS_Store This is a file that Finder on Mac OS loves to add into various folders. --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 04d96ca2c..f7dcea422 100644 --- a/.gitignore +++ b/.gitignore @@ -144,3 +144,6 @@ result # clangd and possibly more .cache/ + +# Mac OS +.DS_Store