From 1570e80219df92461ede2a672f8997a013364f5c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 31 Jul 2023 09:19:19 -0400 Subject: [PATCH 1/6] Move evaluator settings (type and global) to separate file/header --- src/libcmd/common-eval-args.cc | 1 + src/libcmd/installables.cc | 1 + src/libcmd/repl.cc | 1 + src/libexpr/eval-settings.cc | 95 ++++++++++++++++++++++++++ src/libexpr/eval-settings.hh | 98 +++++++++++++++++++++++++++ src/libexpr/eval.cc | 89 +----------------------- src/libexpr/eval.hh | 92 ------------------------- src/libexpr/flake/flake.cc | 1 + src/libexpr/parser.y | 1 + src/libexpr/primops.cc | 1 + src/libexpr/primops/fetchMercurial.cc | 1 + src/libexpr/primops/fetchTree.cc | 1 + src/nix/flake.cc | 1 + src/nix/main.cc | 1 + src/nix/repl.cc | 1 + src/nix/search.cc | 1 + src/nix/upgrade-nix.cc | 1 + 17 files changed, 207 insertions(+), 180 deletions(-) create mode 100644 src/libexpr/eval-settings.cc create mode 100644 src/libexpr/eval-settings.hh diff --git a/src/libcmd/common-eval-args.cc b/src/libcmd/common-eval-args.cc index 3df2c71a5..e36bda52f 100644 --- a/src/libcmd/common-eval-args.cc +++ b/src/libcmd/common-eval-args.cc @@ -1,3 +1,4 @@ +#include "eval-settings.hh" #include "common-eval-args.hh" #include "shared.hh" #include "filetransfer.hh" diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 10b077fb5..9d593a01f 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -11,6 +11,7 @@ #include "derivations.hh" #include "eval-inline.hh" #include "eval.hh" +#include "eval-settings.hh" #include "get-drvs.hh" #include "store-api.hh" #include "shared.hh" diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index f9e9c2bf8..d15162e76 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -26,6 +26,7 @@ extern "C" { #include "eval.hh" #include "eval-cache.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "attr-path.hh" #include "store-api.hh" #include "log-store.hh" diff --git a/src/libexpr/eval-settings.cc b/src/libexpr/eval-settings.cc new file mode 100644 index 000000000..422aaf8d5 --- /dev/null +++ b/src/libexpr/eval-settings.cc @@ -0,0 +1,95 @@ +#include "globals.hh" +#include "profiles.hh" +#include "eval.hh" +#include "eval-settings.hh" + +namespace nix { + +/* Very hacky way to parse $NIX_PATH, which is colon-separated, but + can contain URLs (e.g. "nixpkgs=https://bla...:foo=https://"). */ +static Strings parseNixPath(const std::string & s) +{ + Strings res; + + auto p = s.begin(); + + while (p != s.end()) { + auto start = p; + auto start2 = p; + + while (p != s.end() && *p != ':') { + if (*p == '=') start2 = p + 1; + ++p; + } + + if (p == s.end()) { + if (p != start) res.push_back(std::string(start, p)); + break; + } + + if (*p == ':') { + auto prefix = std::string(start2, s.end()); + if (EvalSettings::isPseudoUrl(prefix) || hasPrefix(prefix, "flake:")) { + ++p; + while (p != s.end() && *p != ':') ++p; + } + res.push_back(std::string(start, p)); + if (p == s.end()) break; + } + + ++p; + } + + return res; +} + +EvalSettings::EvalSettings() +{ + auto var = getEnv("NIX_PATH"); + if (var) nixPath = parseNixPath(*var); +} + +Strings EvalSettings::getDefaultNixPath() +{ + Strings res; + auto add = [&](const Path & p, const std::string & s = std::string()) { + if (pathAccessible(p)) { + if (s.empty()) { + res.push_back(p); + } else { + res.push_back(s + "=" + p); + } + } + }; + + if (!evalSettings.restrictEval && !evalSettings.pureEval) { + add(settings.useXDGBaseDirectories ? getStateDir() + "/nix/defexpr/channels" : getHome() + "/.nix-defexpr/channels"); + add(rootChannelsDir() + "/nixpkgs", "nixpkgs"); + add(rootChannelsDir()); + } + + return res; +} + +bool EvalSettings::isPseudoUrl(std::string_view s) +{ + if (s.compare(0, 8, "channel:") == 0) return true; + size_t pos = s.find("://"); + if (pos == std::string::npos) return false; + std::string scheme(s, 0, pos); + return scheme == "http" || scheme == "https" || scheme == "file" || scheme == "channel" || scheme == "git" || scheme == "s3" || scheme == "ssh"; +} + +std::string EvalSettings::resolvePseudoUrl(std::string_view url) +{ + if (hasPrefix(url, "channel:")) + return "https://nixos.org/channels/" + std::string(url.substr(8)) + "/nixexprs.tar.xz"; + else + return std::string(url); +} + +EvalSettings evalSettings; + +static GlobalConfig::Register rEvalSettings(&evalSettings); + +} diff --git a/src/libexpr/eval-settings.hh b/src/libexpr/eval-settings.hh new file mode 100644 index 000000000..043af6cab --- /dev/null +++ b/src/libexpr/eval-settings.hh @@ -0,0 +1,98 @@ +#pragma once +#include "config.hh" + +namespace nix { + +struct EvalSettings : Config +{ + EvalSettings(); + + static Strings getDefaultNixPath(); + + static bool isPseudoUrl(std::string_view s); + + static std::string resolvePseudoUrl(std::string_view url); + + Setting enableNativeCode{this, false, "allow-unsafe-native-code-during-evaluation", + "Whether builtin functions that allow executing native code should be enabled."}; + + Setting nixPath{ + this, getDefaultNixPath(), "nix-path", + R"( + List of directories to be searched for `<...>` file references + + In particular, outside of [pure evaluation mode](#conf-pure-evaluation), this determines the value of + [`builtins.nixPath`](@docroot@/language/builtin-constants.md#builtins-nixPath). + )"}; + + Setting restrictEval{ + this, false, "restrict-eval", + R"( + If set to `true`, the Nix evaluator will not allow access to any + files outside of the Nix search path (as set via the `NIX_PATH` + environment variable or the `-I` option), or to URIs outside of + [`allowed-uris`](../command-ref/conf-file.md#conf-allowed-uris). + The default is `false`. + )"}; + + Setting pureEval{this, false, "pure-eval", + R"( + Pure evaluation mode ensures that the result of Nix expressions is fully determined by explicitly declared inputs, and not influenced by external state: + + - Restrict file system and network access to files specified by cryptographic hash + - Disable [`bultins.currentSystem`](@docroot@/language/builtin-constants.md#builtins-currentSystem) and [`builtins.currentTime`](@docroot@/language/builtin-constants.md#builtins-currentTime) + )" + }; + + Setting enableImportFromDerivation{ + this, true, "allow-import-from-derivation", + R"( + By default, Nix allows you to `import` from a derivation, allowing + building at evaluation time. With this option set to false, Nix will + throw an error when evaluating an expression that uses this feature, + allowing users to ensure their evaluation will not require any + builds to take place. + )"}; + + Setting allowedUris{this, {}, "allowed-uris", + R"( + A list of URI prefixes to which access is allowed in restricted + evaluation mode. For example, when set to + `https://github.com/NixOS`, builtin functions such as `fetchGit` are + allowed to access `https://github.com/NixOS/patchelf.git`. + )"}; + + Setting traceFunctionCalls{this, false, "trace-function-calls", + R"( + If set to `true`, the Nix evaluator will trace every function call. + Nix will print a log message at the "vomit" level for every function + entrance and function exit. + + function-trace entered undefined position at 1565795816999559622 + function-trace exited undefined position at 1565795816999581277 + function-trace entered /nix/store/.../example.nix:226:41 at 1565795253249935150 + function-trace exited /nix/store/.../example.nix:226:41 at 1565795253249941684 + + The `undefined position` means the function call is a builtin. + + Use the `contrib/stack-collapse.py` script distributed with the Nix + source code to convert the trace logs in to a format suitable for + `flamegraph.pl`. + )"}; + + Setting useEvalCache{this, true, "eval-cache", + "Whether to use the flake evaluation cache."}; + + Setting ignoreExceptionsDuringTry{this, false, "ignore-try", + R"( + If set to true, ignore exceptions inside 'tryEval' calls when evaluating nix expressions in + debug mode (using the --debugger flag). By default the debugger will pause on all exceptions. + )"}; + + Setting traceVerbose{this, false, "trace-verbose", + "Whether `builtins.traceVerbose` should trace its first argument when evaluated."}; +}; + +extern EvalSettings evalSettings; + +} diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index be1bdb806..e57de6c1d 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1,4 +1,5 @@ #include "eval.hh" +#include "eval-settings.hh" #include "hash.hh" #include "types.hh" #include "util.hh" @@ -420,44 +421,6 @@ void initGC() } -/* Very hacky way to parse $NIX_PATH, which is colon-separated, but - can contain URLs (e.g. "nixpkgs=https://bla...:foo=https://"). */ -static Strings parseNixPath(const std::string & s) -{ - Strings res; - - auto p = s.begin(); - - while (p != s.end()) { - auto start = p; - auto start2 = p; - - while (p != s.end() && *p != ':') { - if (*p == '=') start2 = p + 1; - ++p; - } - - if (p == s.end()) { - if (p != start) res.push_back(std::string(start, p)); - break; - } - - if (*p == ':') { - auto prefix = std::string(start2, s.end()); - if (EvalSettings::isPseudoUrl(prefix) || hasPrefix(prefix, "flake:")) { - ++p; - while (p != s.end() && *p != ':') ++p; - } - res.push_back(std::string(start, p)); - if (p == s.end()) break; - } - - ++p; - } - - return res; -} - ErrorBuilder & ErrorBuilder::atPos(PosIdx pos) { info.errPos = state.positions[pos]; @@ -2626,54 +2589,4 @@ std::ostream & operator << (std::ostream & str, const ExternalValueBase & v) { } -EvalSettings::EvalSettings() -{ - auto var = getEnv("NIX_PATH"); - if (var) nixPath = parseNixPath(*var); -} - -Strings EvalSettings::getDefaultNixPath() -{ - Strings res; - auto add = [&](const Path & p, const std::string & s = std::string()) { - if (pathAccessible(p)) { - if (s.empty()) { - res.push_back(p); - } else { - res.push_back(s + "=" + p); - } - } - }; - - if (!evalSettings.restrictEval && !evalSettings.pureEval) { - add(settings.useXDGBaseDirectories ? getStateDir() + "/nix/defexpr/channels" : getHome() + "/.nix-defexpr/channels"); - add(rootChannelsDir() + "/nixpkgs", "nixpkgs"); - add(rootChannelsDir()); - } - - return res; -} - -bool EvalSettings::isPseudoUrl(std::string_view s) -{ - if (s.compare(0, 8, "channel:") == 0) return true; - size_t pos = s.find("://"); - if (pos == std::string::npos) return false; - std::string scheme(s, 0, pos); - return scheme == "http" || scheme == "https" || scheme == "file" || scheme == "channel" || scheme == "git" || scheme == "s3" || scheme == "ssh"; -} - -std::string EvalSettings::resolvePseudoUrl(std::string_view url) -{ - if (hasPrefix(url, "channel:")) - return "https://nixos.org/channels/" + std::string(url.substr(8)) + "/nixexprs.tar.xz"; - else - return std::string(url); -} - -EvalSettings evalSettings; - -static GlobalConfig::Register rEvalSettings(&evalSettings); - - } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 46fa96d05..887b9cb97 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -787,98 +787,6 @@ struct InvalidPathError : EvalError #endif }; -struct EvalSettings : Config -{ - EvalSettings(); - - static Strings getDefaultNixPath(); - - static bool isPseudoUrl(std::string_view s); - - static std::string resolvePseudoUrl(std::string_view url); - - Setting enableNativeCode{this, false, "allow-unsafe-native-code-during-evaluation", - "Whether builtin functions that allow executing native code should be enabled."}; - - Setting nixPath{ - this, getDefaultNixPath(), "nix-path", - R"( - List of directories to be searched for `<...>` file references - - In particular, outside of [pure evaluation mode](#conf-pure-evaluation), this determines the value of - [`builtins.nixPath`](@docroot@/language/builtin-constants.md#builtins-nixPath). - )"}; - - Setting restrictEval{ - this, false, "restrict-eval", - R"( - If set to `true`, the Nix evaluator will not allow access to any - files outside of the Nix search path (as set via the `NIX_PATH` - environment variable or the `-I` option), or to URIs outside of - [`allowed-uris`](../command-ref/conf-file.md#conf-allowed-uris). - The default is `false`. - )"}; - - Setting pureEval{this, false, "pure-eval", - R"( - Pure evaluation mode ensures that the result of Nix expressions is fully determined by explicitly declared inputs, and not influenced by external state: - - - Restrict file system and network access to files specified by cryptographic hash - - Disable [`bultins.currentSystem`](@docroot@/language/builtin-constants.md#builtins-currentSystem) and [`builtins.currentTime`](@docroot@/language/builtin-constants.md#builtins-currentTime) - )" - }; - - Setting enableImportFromDerivation{ - this, true, "allow-import-from-derivation", - R"( - By default, Nix allows you to `import` from a derivation, allowing - building at evaluation time. With this option set to false, Nix will - throw an error when evaluating an expression that uses this feature, - allowing users to ensure their evaluation will not require any - builds to take place. - )"}; - - Setting allowedUris{this, {}, "allowed-uris", - R"( - A list of URI prefixes to which access is allowed in restricted - evaluation mode. For example, when set to - `https://github.com/NixOS`, builtin functions such as `fetchGit` are - allowed to access `https://github.com/NixOS/patchelf.git`. - )"}; - - Setting traceFunctionCalls{this, false, "trace-function-calls", - R"( - If set to `true`, the Nix evaluator will trace every function call. - Nix will print a log message at the "vomit" level for every function - entrance and function exit. - - function-trace entered undefined position at 1565795816999559622 - function-trace exited undefined position at 1565795816999581277 - function-trace entered /nix/store/.../example.nix:226:41 at 1565795253249935150 - function-trace exited /nix/store/.../example.nix:226:41 at 1565795253249941684 - - The `undefined position` means the function call is a builtin. - - Use the `contrib/stack-collapse.py` script distributed with the Nix - source code to convert the trace logs in to a format suitable for - `flamegraph.pl`. - )"}; - - Setting useEvalCache{this, true, "eval-cache", - "Whether to use the flake evaluation cache."}; - - Setting ignoreExceptionsDuringTry{this, false, "ignore-try", - R"( - If set to true, ignore exceptions inside 'tryEval' calls when evaluating nix expressions in - debug mode (using the --debugger flag). By default the debugger will pause on all exceptions. - )"}; - - Setting traceVerbose{this, false, "trace-verbose", - "Whether `builtins.traceVerbose` should trace its first argument when evaluated."}; -}; - -extern EvalSettings evalSettings; - static const std::string corepkgsPrefix{"/__corepkgs__/"}; template diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 9112becff..6a27ea2e8 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -1,5 +1,6 @@ #include "flake.hh" #include "eval.hh" +#include "eval-settings.hh" #include "lockfile.hh" #include "primops.hh" #include "eval-inline.hh" diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 217c17382..201370b90 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -22,6 +22,7 @@ #include "nixexpr.hh" #include "eval.hh" +#include "eval-settings.hh" #include "globals.hh" namespace nix { diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7ff17b6ee..ddf529b9e 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3,6 +3,7 @@ #include "downstream-placeholder.hh" #include "eval-inline.hh" #include "eval.hh" +#include "eval-settings.hh" #include "globals.hh" #include "json-to-value.hh" #include "names.hh" diff --git a/src/libexpr/primops/fetchMercurial.cc b/src/libexpr/primops/fetchMercurial.cc index 322692b52..b9ff01c16 100644 --- a/src/libexpr/primops/fetchMercurial.cc +++ b/src/libexpr/primops/fetchMercurial.cc @@ -1,5 +1,6 @@ #include "primops.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "store-api.hh" #include "fetchers.hh" #include "url.hh" diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 5e668c629..f040a3510 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -1,5 +1,6 @@ #include "primops.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "store-api.hh" #include "fetchers.hh" #include "filetransfer.hh" diff --git a/src/nix/flake.cc b/src/nix/flake.cc index b5f5d0cac..3ce1de44a 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -4,6 +4,7 @@ #include "shared.hh" #include "eval.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "flake/flake.hh" #include "get-drvs.hh" #include "store-api.hh" diff --git a/src/nix/main.cc b/src/nix/main.cc index 650c79d14..df66beb8c 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -3,6 +3,7 @@ #include "command.hh" #include "common-args.hh" #include "eval.hh" +#include "eval-settings.hh" #include "globals.hh" #include "legacy.hh" #include "shared.hh" diff --git a/src/nix/repl.cc b/src/nix/repl.cc index bb14f3f99..9677c1b48 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -1,4 +1,5 @@ #include "eval.hh" +#include "eval-settings.hh" #include "globals.hh" #include "command.hh" #include "installable-value.hh" diff --git a/src/nix/search.cc b/src/nix/search.cc index c92ed1663..ef0139e09 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -2,6 +2,7 @@ #include "globals.hh" #include "eval.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "names.hh" #include "get-drvs.hh" #include "common-args.hh" diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc index d05c23fb7..d238456db 100644 --- a/src/nix/upgrade-nix.cc +++ b/src/nix/upgrade-nix.cc @@ -3,6 +3,7 @@ #include "store-api.hh" #include "filetransfer.hh" #include "eval.hh" +#include "eval-settings.hh" #include "attr-path.hh" #include "names.hh" #include "progress-bar.hh" From 2a5f5fbb1776f5086646407e5296c372321863b9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 31 Jul 2023 11:13:38 -0400 Subject: [PATCH 2/6] `LocalStore::verifyPath`: Use `StorePathSet` for `store` local var We don't care about non-store-paths in there (things like `.links`, are, in fact, allowed). So let's just skip them up front and be more strongly typed. --- src/libstore/local-store.cc | 12 ++++++++---- src/libstore/local-store.hh | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f78bd44ca..9049f33aa 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1499,8 +1499,12 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) auto fdGCLock = openGCLock(); FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock..."); - StringSet store; - for (auto & i : readDirectory(realStoreDir)) store.insert(i.name); + StorePathSet store; + for (auto & i : readDirectory(realStoreDir)) { + try { + store.insert({i.name}); + } catch (BadStorePath &) { } + } /* Check whether all valid paths actually exist. */ printInfo("checking path existence..."); @@ -1595,14 +1599,14 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) } -void LocalStore::verifyPath(const StorePath & path, const StringSet & store, +void LocalStore::verifyPath(const StorePath & path, const StorePathSet & store, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors) { checkInterrupt(); if (!done.insert(path).second) return; - if (!store.count(std::string(path.to_string()))) { + if (!store.count(path)) { /* Check any referrers first. If we can invalidate them first, then we can invalidate this path as well. */ bool canInvalidate = true; diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index c9b570eaa..e97195f5b 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -314,7 +314,7 @@ private: */ void invalidatePathChecked(const StorePath & path); - void verifyPath(const StorePath & path, const StringSet & store, + void verifyPath(const StorePath & path, const StorePathSet & store, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors); std::shared_ptr queryPathInfoInternal(State & state, const StorePath & path); From 6525265f4640221efb0039ddbd6849a3b04babc9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 31 Jul 2023 12:22:06 -0400 Subject: [PATCH 3/6] `LocalStore::verifyPath`: Try to clarify data flow with more scopes It was initially unclear to me which of these are temporary state for the verify paths computation, and which of these are the results of that computation to be used in the rest of the function. Now, it is clear, and enforced. --- src/libstore/local-store.cc | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 9049f33aa..17e2ebc38 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1499,21 +1499,24 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) auto fdGCLock = openGCLock(); FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock..."); - StorePathSet store; - for (auto & i : readDirectory(realStoreDir)) { - try { - store.insert({i.name}); - } catch (BadStorePath &) { } - } - - /* Check whether all valid paths actually exist. */ - printInfo("checking path existence..."); - StorePathSet validPaths; - StorePathSet done; - for (auto & i : queryAllValidPaths()) - verifyPath(i, store, done, validPaths, repair, errors); + { + StorePathSet store; + for (auto & i : readDirectory(realStoreDir)) { + try { + store.insert({i.name}); + } catch (BadStorePath &) { } + } + + /* Check whether all valid paths actually exist. */ + printInfo("checking path existence..."); + + StorePathSet done; + + for (auto & i : queryAllValidPaths()) + verifyPath(i, store, done, validPaths, repair, errors); + } /* Optionally, check the content hashes (slow). */ if (checkContents) { From b9615419688353f4133520dc11456f7653e4cd08 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 31 Jul 2023 22:51:06 +0200 Subject: [PATCH 4/6] labeler: Stop removing labels > Whether or not to remove labels when matching files are reverted or no longer changed by the PR https://github.com/actions/labeler#inputs --- .github/workflows/labels.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 5f949ddc5..d83cb4f18 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -21,4 +21,4 @@ jobs: - uses: actions/labeler@v4 with: repo-token: ${{ secrets.GITHUB_TOKEN }} - sync-labels: true + sync-labels: false From 770d50e49cce4d8ce5e546fe31beaa253505bfa5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 2 Aug 2023 12:40:04 -0400 Subject: [PATCH 5/6] local-store verifying: Rename `store` to something more clear It is not a `Store` but a `StorePathSet`. --- src/libstore/local-store.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 17e2ebc38..982a9059c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1502,10 +1502,10 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) StorePathSet validPaths; { - StorePathSet store; + StorePathSet storePathsInStoreDir; for (auto & i : readDirectory(realStoreDir)) { try { - store.insert({i.name}); + storePathsInStoreDir.insert({i.name}); } catch (BadStorePath &) { } } @@ -1515,7 +1515,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) StorePathSet done; for (auto & i : queryAllValidPaths()) - verifyPath(i, store, done, validPaths, repair, errors); + verifyPath(i, storePathsInStoreDir, done, validPaths, repair, errors); } /* Optionally, check the content hashes (slow). */ @@ -1602,21 +1602,21 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) } -void LocalStore::verifyPath(const StorePath & path, const StorePathSet & store, +void LocalStore::verifyPath(const StorePath & path, const StorePathSet & storePathsInStoreDir, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors) { checkInterrupt(); if (!done.insert(path).second) return; - if (!store.count(path)) { + if (!storePathsInStoreDir.count(path)) { /* Check any referrers first. If we can invalidate them first, then we can invalidate this path as well. */ bool canInvalidate = true; StorePathSet referrers; queryReferrers(path, referrers); for (auto & i : referrers) if (i != path) { - verifyPath(i, store, done, validPaths, repair, errors); + verifyPath(i, storePathsInStoreDir, done, validPaths, repair, errors); if (validPaths.count(i)) canInvalidate = false; } From 66550878dffe2a4bf55ea7ab694738aa14e6a01e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 2 Aug 2023 12:45:55 -0400 Subject: [PATCH 6/6] Add comment explaining the use of `readDirectory(realStoreDir)` --- src/libstore/local-store.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 982a9059c..40a3bc194 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1503,6 +1503,15 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) { StorePathSet storePathsInStoreDir; + /* Why aren't we using `queryAllValidPaths`? Because that would + tell us about all the paths than the database knows about. Here we + want to know about all the store paths in the store directory, + regardless of what the database thinks. + + We will end up cross-referencing these two sources of truth (the + database and the filesystem) in the loop below, in order to catch + invalid states. + */ for (auto & i : readDirectory(realStoreDir)) { try { storePathsInStoreDir.insert({i.name});