libstore: fix reporting output cycles on drvs with references to other drvs

Closes #1064

The culprit here is that `genGraphString` is only invoked with the
store-paths associated with the outputs of the derivation, so when
filling `dependents`, the `graph_data.find(p)` call would return the end
of the iterator when doing this for references to other store-paths.

As a result, the code wrote information behind the graph data-structure
causing a corruption. For me, this resulted in a SIGSEGV most of the
time and in a few cases in an uncaught `map::at`-exception as reported
by Niklas.

This patch changes two aspects of the original implementation:

* When filling `dependents` in the node-set, use `map.at()` instead of
  `map.find()->second`. The latter doesn't make any sense and was the
  cause of corrupting memory. The `at` would've made it far easier to
  spot this in the first place.

* Filter out store-paths that don't belong to a different output of the
  derivation when creating `outputGraph`. This variable is used on two
  places, `genGraphString` and for topological sorting.

  The latter already filters out store-paths from a different drv, so
  this is happening now when creating the variable in the first place
  such that `genGraphString` never ends up with corrupt data in the
  first place. This is the actual bugfix.

Implemented a regression-test for this case to be sure.

Change-Id: Ie02144d89c32b0a776cb1ece0601d0229315ebc3

lix/libstore/build/local-derivation-goal.cc

@@ -1832,15 +1832,18 @@ try {
             );
         }
 
+        outputGraph[scratchOutputs.at(name)] = StorePathSet{};
         std::visit(
             overloaded{/* Since we'll use the already installed versions of these, we
                                    can treat them as leaves and ignore any references they
                                    have. */
-                       [&](const AlreadyRegistered &) {
-                           outputGraph[scratchOutputs.at(name)] = StorePathSet{};
-                       },
+                       [&](const AlreadyRegistered &) {},
                        [&](const PerhapsNeedToRegister & refs) {
-                           outputGraph[scratchOutputs.at(name)] = refs.refs;
+                           for (auto & ref : refs.refs) {
+                               if (inverseOutputMap.find(ref) != inverseOutputMap.end()) {
+                                   outputGraph[scratchOutputs.at(name)].insert(ref);
+                               }
+                           }
                        }
             },
             *orifu
@@ -1851,10 +1854,8 @@ try {
         topoSort(outputsToSort, {[&](const std::string & name) {
                      StringSet dependencies;
                      for (auto & path : outputGraph.at(scratchOutputs.at(name))) {
-                         auto outputName = inverseOutputMap.find(path);
-                         if (outputName != inverseOutputMap.end()) {
-                             dependencies.insert(outputName->second);
-                         }
+                         auto outputName = inverseOutputMap.at(path);
+                         dependencies.insert(outputName);
                      }
                      return dependencies;
                  }});

lix/libstore/path-tree.cc

@@ -265,7 +265,7 @@ static std::map<StorePath, Node> mkGraph(
 
     for (auto & node : graph_data) {
         for (auto & ref : node.second.dependencies) {
-            graph_data.find(ref)->second.dependents.insert(node.first);
+            graph_data.at(ref).dependents.insert(node.first);
         }
     }
 

tests/functional/check-outputs.nix

@@ -15,6 +15,18 @@ rec {
     '';
   };
 
+  cycle-with-deps = mkDerivation {
+    name = "cycle-with-deps";
+    inherit dep;
+    outputs = [ "foo" "bar" ];
+    builder = builtins.toFile "builder.sh" ''
+      mkdir -p $foo/bin $bar/lib
+      ln -sf $dep $bar/lib
+      echo $foo > $bar/txt
+      echo $bar > $foo/txt
+    '';
+  };
+
   as_dependency = mkDerivation {
     name = "depends-on-cycle";
     inherit cycle;

tests/functional/output-cycles.sh

@@ -16,3 +16,14 @@ error="$(! nix-build check-outputs.nix -A as_dependency 2>&1)"
 
 grepQuiet "cycle detected in build of '.*' in the references of output 'bar' from output 'foo'" <<<"$error"
 grepQuiet "error: 1 dependencies of derivation" <<<"$error"
+
+error="$(! nix-build check-outputs.nix -A cycle-with-deps 2>&1)"
+grepQuiet "cycle detected in build of '.*' in the references of output 'bar' from output 'foo'" <<<"$error"
+
+if [[ "$(uname -s)" = Linux ]]; then
+    echo "$error"
+    <<<"$error" grepQuiet "/store/.*-cycle-with-deps-bar"
+    <<<"$error" grepQuiet "└───txt: ….*cycle-with-deps-foo.*"
+    <<<"$error" grepQuiet "    →.*/store/.*-cycle-with-deps-foo"
+    <<<"$error" grepQuiet "    └───txt:.*-cycle-with-deps-bar.*"
+fi