Auto merge of #5988 - Eh2406:explore_the_bug, r=alexcrichton

BUG fuzzing found a bug in the resolver, we need a complete set of conflicts to do backjumping

As mentioned in #5921 (comment), the new proptest found a live bug! This PR so far tracs my attempt to minimize the problematic input.

The problem turned out to be that we where backjumping on incomplete set of conflicts.

Author: bors

Cargo.toml

@@ -22,7 +22,7 @@ crates-io = { path = "src/crates-io", version = "0.19" }
 crossbeam-utils = "0.5"
 crypto-hash = "0.3.1"
 curl = "0.4.13"
-env_logger = "0.5.4"
+env_logger = "0.5.11"
 failure = "0.1.2"
 filetime = "0.2"
 flate2 = "1.0"

src/cargo/core/resolver/conflict_cache.rs

@@ -1,8 +1,8 @@
 use std::collections::{HashMap, HashSet};
 
-use core::{Dependency, PackageId};
-use core::resolver::Context;
 use super::types::ConflictReason;
+use core::resolver::Context;
+use core::{Dependency, PackageId};
 
 pub(super) struct ConflictCache {
     // `con_from_dep` is a cache of the reasons for each time we
@@ -77,11 +77,17 @@ impl ConflictCache {
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated
     pub fn insert(&mut self, dep: &Dependency, con: &HashMap<PackageId, ConflictReason>) {
-        let past = self.con_from_dep
+        let past = self
+            .con_from_dep
             .entry(dep.clone())
             .or_insert_with(Vec::new);
         if !past.contains(con) {
-            trace!("{} adding a skip {:?}", dep.package_name(), con);
+            trace!(
+                "{} = \"{}\" adding a skip {:?}",
+                dep.package_name(),
+                dep.version_req(),
+                con
+            );
             past.push(con.clone());
             for c in con.keys() {
                 self.dep_from_pid

src/cargo/core/resolver/mod.rs

@@ -305,7 +305,12 @@ fn activate_deps_loop(
                 // It's our job here to backtrack, if possible, and find a
                 // different candidate to activate. If we can't find any
                 // candidates whatsoever then it's time to bail entirely.
-                trace!("{}[{}]>{} -- no candidates", parent.name(), cur, dep.package_name());
+                trace!(
+                    "{}[{}]>{} -- no candidates",
+                    parent.name(),
+                    cur,
+                    dep.package_name()
+                );
 
                 // Use our list of `conflicting_activations` to add to our
                 // global list of past conflicting activations, effectively
@@ -325,7 +330,12 @@ fn activate_deps_loop(
                     past_conflicting_activations.insert(&dep, &conflicting_activations);
                 }
 
-                match find_candidate(&mut backtrack_stack, &parent, &conflicting_activations) {
+                match find_candidate(
+                    &mut backtrack_stack,
+                    &parent,
+                    backtracked,
+                    &conflicting_activations,
+                ) {
                     Some((candidate, has_another, frame)) => {
                         // Reset all of our local variables used with the
                         // contents of `frame` to complete our backtrack.
@@ -432,8 +442,7 @@ fn activate_deps_loop(
                             .clone()
                             .filter_map(|(_, (ref new_dep, _, _))| {
                                 past_conflicting_activations.conflicting(&cx, new_dep)
-                            })
-                            .next()
+                            }).next()
                         {
                             // If one of our deps is known unresolvable
                             // then we will not succeed.
@@ -467,18 +476,14 @@ fn activate_deps_loop(
                                 .iter()
                                 .flat_map(|other| other.flatten())
                                 // for deps related to us
-                                .filter(|&(_, ref other_dep)|
-                                        known_related_bad_deps.contains(other_dep))
-                                .filter_map(|(other_parent, other_dep)| {
+                                .filter(|&(_, ref other_dep)| {
+                                    known_related_bad_deps.contains(other_dep)
+                                }).filter_map(|(other_parent, other_dep)| {
                                     past_conflicting_activations
-                                        .find_conflicting(
-                                            &cx,
-                                            &other_dep,
-                                            |con| con.contains_key(&pid)
-                                        )
-                                        .map(|con| (other_parent, con))
-                                })
-                                .next()
+                                        .find_conflicting(&cx, &other_dep, |con| {
+                                            con.contains_key(&pid)
+                                        }).map(|con| (other_parent, con))
+                                }).next()
                             {
                                 let rel = conflict.get(&pid).unwrap().clone();
 
@@ -518,6 +523,7 @@ fn activate_deps_loop(
                             find_candidate(
                                 &mut backtrack_stack.clone(),
                                 &parent,
+                                backtracked,
                                 &conflicting_activations,
                             ).is_none()
                         }
@@ -809,6 +815,7 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
 fn find_candidate(
     backtrack_stack: &mut Vec<BacktrackFrame>,
     parent: &Summary,
+    backtracked: bool,
     conflicting_activations: &HashMap<PackageId, ConflictReason>,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
@@ -830,11 +837,20 @@ fn find_candidate(
         // active in this back up we know that we're guaranteed to not actually
         // make any progress. As a result if we hit this condition we can
         // completely skip this backtrack frame and move on to the next.
-        if frame
-            .context_backup
-            .is_conflicting(Some(parent.package_id()), conflicting_activations)
-        {
-            continue;
+        if !backtracked {
+            if frame
+                .context_backup
+                .is_conflicting(Some(parent.package_id()), conflicting_activations)
+            {
+                trace!(
+                    "{} = \"{}\" skip as not solving {}: {:?}",
+                    frame.dep.package_name(),
+                    frame.dep.version_req(),
+                    parent.package_id(),
+                    conflicting_activations
+                );
+                continue;
+            }
         }
 
         return Some((candidate, has_another, frame));

tests/testsuite/resolve.rs

@@ -104,7 +104,7 @@ impl<'a> ToPkgId for (&'a str, String) {
 }
 
 macro_rules! pkg {
-    ($pkgid:expr => [$($deps:expr),+]) => ({
+    ($pkgid:expr => [$($deps:expr),+ $(,)* ]) => ({
         let d: Vec<Dependency> = vec![$($deps.to_dep()),+];
         let pkgid = $pkgid.to_pkgid();
         let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None};
@@ -963,6 +963,174 @@ fn resolving_with_constrained_sibling_transitive_dep_effects() {
     );
 }
 
+#[test]
+fn incomplete_information_skiping() {
+    // When backtracking due to a failed dependency, if Cargo is
+    // trying to be clever and skip irrelevant dependencies, care must
+    // be taken to not miss the transitive effects of alternatives.
+    // Fuzzing discovered that for some reason cargo was skiping based
+    // on incomplete information in the following case:
+    // minimized bug found in:
+    // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9
+    let input = vec![
+        pkg!(("a", "1.0.0")),
+        pkg!(("a", "1.1.0")),
+        pkg!("b" => [dep("a")]),
+        pkg!(("c", "1.0.0")),
+        pkg!(("c", "1.1.0")),
+        pkg!("d" => [dep_req("c", "=1.0")]),
+        pkg!(("e", "1.0.0")),
+        pkg!(("e", "1.1.0") => [dep_req("c", "1.1")]),
+        pkg!("to_yank"),
+        pkg!(("f", "1.0.0") => [
+            dep("to_yank"),
+            dep("d"),
+        ]),
+        pkg!(("f", "1.1.0") => [dep("d")]),
+        pkg!("g" => [
+            dep("b"),
+            dep("e"),
+            dep("f"),
+        ]),
+    ];
+    let reg = registry(input.clone());
+
+    let res = resolve(&pkg_id("root"), vec![dep("g")], &reg).unwrap();
+    let package_to_yank = "to_yank".to_pkgid();
+    // this package is not used in the resolution.
+    assert!(!res.contains(&package_to_yank));
+    // so when we yank it
+    let new_reg = registry(
+        input
+            .iter()
+            .cloned()
+            .filter(|x| &package_to_yank != x.package_id())
+            .collect(),
+    );
+    assert_eq!(input.len(), new_reg.len() + 1);
+    // it should still build
+    assert!(resolve(&pkg_id("root"), vec![dep("g")], &new_reg).is_ok());
+}
+
+#[test]
+fn incomplete_information_skiping_2() {
+    // When backtracking due to a failed dependency, if Cargo is
+    // trying to be clever and skip irrelevant dependencies, care must
+    // be taken to not miss the transitive effects of alternatives.
+    // Fuzzing discovered that for some reason cargo was skiping based
+    // on incomplete information in the following case:
+    // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9
+    let input = vec![
+        pkg!(("b", "3.8.10")),
+        pkg!(("b", "8.7.4")),
+        pkg!(("b", "9.4.6")),
+        pkg!(("c", "1.8.8")),
+        pkg!(("c", "10.2.5")),
+        pkg!(("d", "4.1.2") => [
+            dep_req("bad", "=6.10.9"),
+        ]),
+        pkg!(("d", "5.5.6")),
+        pkg!(("d", "5.6.10")),
+        pkg!(("to_yank", "8.0.1")),
+        pkg!(("to_yank", "8.8.1")),
+        pkg!(("e", "4.7.8") => [
+            dep_req("d", ">=5.5.6, <=5.6.10"),
+            dep_req("to_yank", "=8.0.1"),
+        ]),
+        pkg!(("e", "7.4.9") => [
+            dep_req("bad", "=4.7.5"),
+        ]),
+        pkg!("f" => [
+            dep_req("d", ">=4.1.2, <=5.5.6"),
+        ]),
+        pkg!("g" => [
+            dep("bad"),
+        ]),
+        pkg!(("h", "3.8.3") => [
+            dep_req("g", "*"),
+        ]),
+        pkg!(("h", "6.8.3") => [
+            dep("f"),
+        ]),
+        pkg!(("h", "8.1.9") => [
+            dep_req("to_yank", "=8.8.1"),
+        ]),
+        pkg!("i" => [
+            dep_req("b", "*"),
+            dep_req("c", "*"),
+            dep_req("e", "*"),
+            dep_req("h", "*"),
+        ]),
+    ];
+    let reg = registry(input.clone());
+
+    let res = resolve(&pkg_id("root"), vec![dep("i")], &reg).unwrap();
+    let package_to_yank = ("to_yank", "8.8.1").to_pkgid();
+    // this package is not used in the resolution.
+    assert!(!res.contains(&package_to_yank));
+    // so when we yank it
+    let new_reg = registry(
+        input
+            .iter()
+            .cloned()
+            .filter(|x| &package_to_yank != x.package_id())
+            .collect(),
+    );
+    assert_eq!(input.len(), new_reg.len() + 1);
+    // it should still build
+    assert!(resolve(&pkg_id("root"), vec![dep("i")], &new_reg).is_ok());
+}
+
+#[test]
+fn incomplete_information_skiping_3() {
+    // When backtracking due to a failed dependency, if Cargo is
+    // trying to be clever and skip irrelevant dependencies, care must
+    // be taken to not miss the transitive effects of alternatives.
+    // Fuzzing discovered that for some reason cargo was skiping based
+    // on incomplete information in the following case:
+    // minimized bug found in:
+    // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9
+    let input = vec![
+        pkg!{("to_yank", "3.0.3")},
+        pkg!{("to_yank", "3.3.0")},
+        pkg!{("to_yank", "3.3.1")},
+        pkg!{("a", "3.3.0") => [
+            dep_req("to_yank", "=3.0.3"),
+        ] },
+        pkg!{("a", "3.3.2") => [
+            dep_req("to_yank", "<=3.3.0"),
+        ] },
+        pkg!{("b", "0.1.3") => [
+            dep_req("a", "=3.3.0"),
+        ] },
+        pkg!{("b", "2.0.2") => [
+            dep_req("to_yank", "3.3.0"),
+            dep_req("a", "*"),
+        ] },
+        pkg!{("b", "2.3.3") => [
+            dep_req("to_yank", "3.3.0"),
+            dep_req("a", "=3.3.0"),
+        ] },
+    ];
+    let reg = registry(input.clone());
+
+    let res = resolve(&pkg_id("root"), vec![dep_req("b", "*")], &reg).unwrap();
+    let package_to_yank = ("to_yank", "3.0.3").to_pkgid();
+    // this package is not used in the resolution.
+    assert!(!res.contains(&package_to_yank));
+    // so when we yank it
+    let new_reg = registry(
+        input
+            .iter()
+            .cloned()
+            .filter(|x| &package_to_yank != x.package_id())
+            .collect(),
+    );
+    assert_eq!(input.len(), new_reg.len() + 1);
+    // it should still build
+    assert!(resolve(&pkg_id("root"), vec![dep_req("b", "*")], &new_reg).is_ok());
+}
+
 #[test]
 fn resolving_but_no_exists() {
     let reg = registry(vec![]);