From f923ed6b6a7318e8fc77e8d3aeda6796671f67cb Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 22 May 2024 12:35:44 -0400 Subject: [PATCH] Require `drvPath` attribute to end with `.drv` Fixes #4977 --- src/libexpr/eval-cache.cc | 1 + src/libexpr/eval-error.cc | 3 +-- src/libexpr/get-drvs.cc | 18 +++++++++---- src/libexpr/print.cc | 23 +++++++++++----- src/libstore/derivations.cc | 5 ++-- src/libstore/path.cc | 8 +++++- src/libstore/path.hh | 27 +++++++------------ src/nix-env/user-env.cc | 1 + src/nix/bundle.cc | 2 ++ tests/functional/lang.sh | 3 +++ .../lang/non-eval-fail-bad-drvPath.nix | 14 ++++++++++ 11 files changed, 71 insertions(+), 34 deletions(-) create mode 100644 tests/functional/lang/non-eval-fail-bad-drvPath.nix diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index d60967a14..a8222b985 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -753,6 +753,7 @@ StorePath AttrCursor::forceDerivation() { auto aDrvPath = getAttr(root->state.sDrvPath, true); auto drvPath = root->state.store->parseStorePath(aDrvPath->getString()); + drvPath.requireDerivation(); if (!root->state.store->isValidPath(drvPath) && !settings.readOnlyMode) { /* The eval cache contains 'drvPath', but the actual path has been garbage-collected. So force it to be regenerated. */ diff --git a/src/libexpr/eval-error.cc b/src/libexpr/eval-error.cc index 8db03610b..a9409468c 100644 --- a/src/libexpr/eval-error.cc +++ b/src/libexpr/eval-error.cc @@ -27,8 +27,7 @@ EvalErrorBuilder & EvalErrorBuilder::atPos(Value & value, PosIdx fallback) template EvalErrorBuilder & EvalErrorBuilder::withTrace(PosIdx pos, const std::string_view text) { - error.err.traces.push_front( - Trace{.pos = error.state.positions[pos], .hint = HintFmt(std::string(text))}); + error.addTrace(error.state.positions[pos], text); return *this; } diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index cf10ed84a..ed16a51a1 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -69,13 +69,21 @@ std::string PackageInfo::querySystem() const std::optional PackageInfo::queryDrvPath() const { if (!drvPath && attrs) { - NixStringContext context; - if (auto i = attrs->get(state->sDrvPath)) - drvPath = {state->coerceToStorePath(i->pos, *i->value, context, "while evaluating the 'drvPath' attribute of a derivation")}; - else + if (auto i = attrs->get(state->sDrvPath)) { + NixStringContext context; + auto found = state->coerceToStorePath(i->pos, *i->value, context, "while evaluating the 'drvPath' attribute of a derivation"); + try { + found.requireDerivation(); + } catch (Error & e) { + e.addTrace(state->positions[i->pos], "while evaluating the 'drvPath' attribute of a derivation"); + throw; + } + drvPath = {std::move(found)}; + } else drvPath = {std::nullopt}; } - return drvPath.value_or(std::nullopt); + drvPath.value_or(std::nullopt); + return *drvPath; } diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 7799a0bbe..d53fadac7 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -271,16 +271,27 @@ private: void printDerivation(Value & v) { - NixStringContext context; - std::string storePath; - if (auto i = v.attrs()->get(state.sDrvPath)) - storePath = state.store->printStorePath(state.coerceToStorePath(i->pos, *i->value, context, "while evaluating the drvPath of a derivation")); + std::optional storePath; + if (auto i = v.attrs()->get(state.sDrvPath)) { + NixStringContext context; + storePath = state.coerceToStorePath(i->pos, *i->value, context, "while evaluating the drvPath of a derivation"); + } + + /* This unforutately breaks printing nested values because of + how the pretty printer is used (when pretting printing and warning + to same terminal / std stream). */ +#if 0 + if (storePath && !storePath->isDerivation()) + warn( + "drvPath attribute '%s' is not a valid store path to a derivation, this value not work properly", + state.store->printStorePath(*storePath)); +#endif if (options.ansiColors) output << ANSI_GREEN; output << "«derivation"; - if (!storePath.empty()) { - output << " " << storePath; + if (storePath) { + output << " " << state.store->printStorePath(*storePath); } output << "»"; if (options.ansiColors) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index e13705911..869880112 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -930,10 +930,9 @@ DerivationOutputsAndOptPaths BasicDerivation::outputsAndOptPaths(const StoreDirC std::string_view BasicDerivation::nameFromPath(const StorePath & drvPath) { + drvPath.requireDerivation(); auto nameWithSuffix = drvPath.name(); - constexpr std::string_view extension = ".drv"; - assert(hasSuffix(nameWithSuffix, extension)); - nameWithSuffix.remove_suffix(extension.size()); + nameWithSuffix.remove_suffix(drvExtension.size()); return nameWithSuffix; } diff --git a/src/libstore/path.cc b/src/libstore/path.cc index 4b806e408..8d9726722 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -49,11 +49,17 @@ StorePath::StorePath(const Hash & hash, std::string_view _name) checkName(baseName, name()); } -bool StorePath::isDerivation() const +bool StorePath::isDerivation() const noexcept { return hasSuffix(name(), drvExtension); } +void StorePath::requireDerivation() const +{ + if (!isDerivation()) + throw FormatError("store path '%s' is not a valid derivation path", to_string()); +} + StorePath StorePath::dummy("ffffffffffffffffffffffffffffffff-x"); StorePath StorePath::random(std::string_view name) diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 3c26fc515..4abbfcd7c 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -35,30 +35,23 @@ public: StorePath(const Hash & hash, std::string_view name); - std::string_view to_string() const + std::string_view to_string() const noexcept { return baseName; } - bool operator < (const StorePath & other) const - { - return baseName < other.baseName; - } - - bool operator == (const StorePath & other) const - { - return baseName == other.baseName; - } - - bool operator != (const StorePath & other) const - { - return baseName != other.baseName; - } + bool operator == (const StorePath & other) const noexcept = default; + auto operator <=> (const StorePath & other) const noexcept = default; /** * Check whether a file name ends with the extension for derivations. */ - bool isDerivation() const; + bool isDerivation() const noexcept; + + /** + * Throw an exception if `isDerivation` is false. + */ + void requireDerivation() const; std::string_view name() const { @@ -82,7 +75,7 @@ typedef std::vector StorePaths; * The file extension of \ref Derivation derivations when serialized * into store objects. */ -const std::string drvExtension = ".drv"; +constexpr std::string_view drvExtension = ".drv"; } diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index 6cbbacb15..f7b091f8f 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -140,6 +140,7 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, NixStringContext context; auto & aDrvPath(*topLevel.attrs()->find(state.sDrvPath)); auto topLevelDrv = state.coerceToStorePath(aDrvPath.pos, *aDrvPath.value, context, ""); + topLevelDrv.requireDerivation(); auto & aOutPath(*topLevel.attrs()->find(state.sOutPath)); auto topLevelOut = state.coerceToStorePath(aOutPath.pos, *aOutPath.value, context, ""); diff --git a/src/nix/bundle.cc b/src/nix/bundle.cc index 2e50392f7..554c36540 100644 --- a/src/nix/bundle.cc +++ b/src/nix/bundle.cc @@ -100,6 +100,8 @@ struct CmdBundle : InstallableValueCommand NixStringContext context2; auto drvPath = evalState->coerceToStorePath(attr1->pos, *attr1->value, context2, ""); + drvPath.requireDerivation(); + auto attr2 = vRes->attrs()->get(evalState->sOutPath); if (!attr2) throw Error("the bundler '%s' does not produce a derivation", bundler.what()); diff --git a/tests/functional/lang.sh b/tests/functional/lang.sh index c45326473..60603cebe 100755 --- a/tests/functional/lang.sh +++ b/tests/functional/lang.sh @@ -24,6 +24,9 @@ nix-instantiate --eval -E 'builtins.traceVerbose "Hello" 123' 2>&1 | grepQuietIn nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" 123' 2>&1 | grepQuietInverse Hello expectStderr 1 nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" (throw "Foo")' | grepQuiet Hello expectStderr 1 nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello %" (throw "Foo")' | grepQuiet 'Hello %' +# Relies on parsing the expression derivation as a derivation, can't use --eval +expectStderr 1 nix-instantiate --show-trace lang/non-eval-fail-bad-drvPath.nix | grepQuiet "store path '8qlfcic10lw5304gqm8q45nr7g7jl62b-cachix-1.7.3-bin' is not a valid derivation path" + nix-instantiate --eval -E 'let x = builtins.trace { x = x; } true; in x' \ 2>&1 | grepQuiet -E 'trace: { x = «potential infinite recursion»; }' diff --git a/tests/functional/lang/non-eval-fail-bad-drvPath.nix b/tests/functional/lang/non-eval-fail-bad-drvPath.nix new file mode 100644 index 000000000..23639bc54 --- /dev/null +++ b/tests/functional/lang/non-eval-fail-bad-drvPath.nix @@ -0,0 +1,14 @@ +let + package = { + type = "derivation"; + name = "cachix-1.7.3"; + system = builtins.currentSystem; + outputs = [ "out" ]; + # Illegal, because does not end in `.drv` + drvPath = "${builtins.storeDir}/8qlfcic10lw5304gqm8q45nr7g7jl62b-cachix-1.7.3-bin"; + outputName = "out"; + outPath = "${builtins.storeDir}/8qlfcic10lw5304gqm8q45nr7g7jl62b-cachix-1.7.3-bin"; + out = package; + }; +in +package