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.

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![]);