cargo tree: Fix stack overflow on cyclic features.

Author: ehuss

src/cargo/ops/tree/graph.rs

@@ -427,28 +427,32 @@ fn add_pkg(
 /// ```text
 /// from -Edge-> featname -Edge::Feature-> to
 /// ```
+///
+/// Returns a tuple `(missing, index)`.
+/// `missing` is true if this feature edge was already added.
+/// `index` is the index of the index in the graph of the `Feature` node.
 fn add_feature(
     graph: &mut Graph<'_>,
     name: InternedString,
     from: Option<usize>,
     to: usize,
     kind: EdgeKind,
-) -> usize {
+) -> (bool, usize) {
     // `to` *must* point to a package node.
     assert!(matches! {graph.nodes[to], Node::Package{..}});
     let node = Node::Feature {
         node_index: to,
         name,
     };
-    let node_index = match graph.index.get(&node) {
-        Some(idx) => *idx,
-        None => graph.add_node(node),
+    let (missing, node_index) = match graph.index.get(&node) {
+        Some(idx) => (false, *idx),
+        None => (true, graph.add_node(node)),
     };
     if let Some(from) = from {
         graph.edges[from].add_edge(kind, node_index);
     }
     graph.edges[node_index].add_edge(EdgeKind::Feature, to);
-    node_index
+    (missing, node_index)
 }
 
 /// Adds nodes for features requested on the command-line for the given member.
@@ -480,7 +484,7 @@ fn add_cli_features(
     for fv in to_add {
         match fv {
             FeatureValue::Feature(feature) => {
-                let index = add_feature(graph, feature, None, package_index, EdgeKind::Feature);
+                let index = add_feature(graph, feature, None, package_index, EdgeKind::Feature).1;
                 graph.cli_features.insert(index);
             }
             // This is enforced by CliFeatures.
@@ -511,10 +515,11 @@ fn add_cli_features(
                     if is_optional {
                         // Activate the optional dep on self.
                         let index =
-                            add_feature(graph, dep_name, None, package_index, EdgeKind::Feature);
+                            add_feature(graph, dep_name, None, package_index, EdgeKind::Feature).1;
                         graph.cli_features.insert(index);
                     }
-                    let index = add_feature(graph, dep_feature, None, dep_index, EdgeKind::Feature);
+                    let index =
+                        add_feature(graph, dep_feature, None, dep_index, EdgeKind::Feature).1;
                     graph.cli_features.insert(index);
                 }
             }
@@ -571,21 +576,24 @@ fn add_feature_rec(
     for fv in fvs {
         match fv {
             FeatureValue::Feature(dep_name) => {
-                let feat_index = add_feature(
+                let (missing, feat_index) = add_feature(
                     graph,
                     *dep_name,
                     Some(from),
                     package_index,
                     EdgeKind::Feature,
                 );
-                add_feature_rec(
-                    graph,
-                    resolve,
-                    *dep_name,
-                    package_id,
-                    feat_index,
-                    package_index,
-                );
+                // Don't recursive if the edge already exists to deal with cycles.
+                if missing {
+                    add_feature_rec(
+                        graph,
+                        resolve,
+                        *dep_name,
+                        package_id,
+                        feat_index,
+                        package_index,
+                    );
+                }
             }
             // Dependencies are already shown in the graph as dep edges. I'm
             // uncertain whether or not this might be confusing in some cases
@@ -628,21 +636,23 @@ fn add_feature_rec(
                             EdgeKind::Feature,
                         );
                     }
-                    let feat_index = add_feature(
+                    let (missing, feat_index) = add_feature(
                         graph,
                         *dep_feature,
                         Some(from),
                         dep_index,
                         EdgeKind::Feature,
                     );
-                    add_feature_rec(
-                        graph,
-                        resolve,
-                        *dep_feature,
-                        dep_pkg_id,
-                        feat_index,
-                        dep_index,
-                    );
+                    if missing {
+                        add_feature_rec(
+                            graph,
+                            resolve,
+                            *dep_feature,
+                            dep_pkg_id,
+                            feat_index,
+                            dep_index,
+                        );
+                    }
                 }
             }
         }

tests/testsuite/tree.rs

@@ -1831,3 +1831,109 @@ foo v0.1.0 ([..]/foo)
         .with_status(101)
         .run();
 }
+
+#[cargo_test]
+fn cyclic_features() {
+    // Check for stack overflow with cyclic features (oops!).
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "1.0.0"
+
+                [features]
+                a = ["b"]
+                b = ["a"]
+                default = ["a"]
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("tree -e features")
+        .with_stdout("foo v1.0.0 ([ROOT]/foo)")
+        .run();
+
+    p.cargo("tree -e features -i foo")
+        .with_stdout(
+            "\
+foo v1.0.0 ([ROOT]/foo)
+├── foo feature \"a\"
+│   ├── foo feature \"b\"
+│   │   └── foo feature \"a\" (*)
+│   └── foo feature \"default\" (command-line)
+├── foo feature \"b\" (*)
+└── foo feature \"default\" (command-line)
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn dev_dep_cycle_with_feature() {
+    // Cycle with features and a dev-dependency.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "1.0.0"
+
+                [dev-dependencies]
+                bar = { path = "bar" }
+
+                [features]
+                a = ["bar/feat1"]
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "1.0.0"
+
+                [dependencies]
+                foo = { path = ".." }
+
+                [features]
+                feat1 = ["foo/a"]
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("tree -e features --features a")
+        .with_stdout(
+            "\
+foo v1.0.0 ([ROOT]/foo)
+[dev-dependencies]
+└── bar feature \"default\"
+    └── bar v1.0.0 ([ROOT]/foo/bar)
+        └── foo feature \"default\" (command-line)
+            └── foo v1.0.0 ([ROOT]/foo) (*)
+",
+        )
+        .run();
+
+    p.cargo("tree -e features --features a -i foo")
+        .with_stdout(
+            "\
+foo v1.0.0 ([ROOT]/foo)
+├── foo feature \"a\" (command-line)
+│   └── bar feature \"feat1\"
+│       └── foo feature \"a\" (command-line) (*)
+└── foo feature \"default\" (command-line)
+    └── bar v1.0.0 ([ROOT]/foo/bar)
+        ├── bar feature \"default\"
+        │   [dev-dependencies]
+        │   └── foo v1.0.0 ([ROOT]/foo) (*)
+        └── bar feature \"feat1\" (*)
+",
+        )
+        .run();
+}