Merge pull request #10757 from obsidiansystems/fix-4977

Require `drvPath` attribute to end with `.drv`

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. */

src/libexpr/eval-error.cc

@@ -27,8 +27,7 @@ EvalErrorBuilder<T> & EvalErrorBuilder<T>::atPos(Value & value, PosIdx fallback)
 template<class T>
 EvalErrorBuilder<T> & EvalErrorBuilder<T>::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;
 }
 

src/libexpr/get-drvs.cc

@@ -69,13 +69,21 @@ std::string PackageInfo::querySystem() const
 std::optional<StorePath> 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;
 }
 
 

src/libexpr/print.cc

@@ -271,16 +271,27 @@ class Printer
 
     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> 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)

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;
 }
 

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)

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<StorePath> StorePaths;
  * The file extension of \ref Derivation derivations when serialized
  * into store objects.
  */
-const std::string drvExtension = ".drv";
+constexpr std::string_view drvExtension = ".drv";
 
 }
 

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, "");
 

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());

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»; }'

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