Fix an issue of deadlock in Cargo

Currently Cargo can deadlock itself via file locks in somewhat obscure
scenarios. One way to trigger this scenario is by causing the index to fail
being created, for example through an invalid `init.templatedir` configuration.

This commit takes the strategy of solving two bugs:

* First, the deadlock is fixed. This is done by manually ensuring that the
  current deadlock doesn't happen by scheduling cached work to happen outside
  the scope of a lock.
* Second, the initialization of the registry's index is fixed. We turn off the
  usage of external templates as we don't want to use them for this internally
  managed repository anyway.

Closes #5551

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 904179c7a200b51d490ae3b7009f6781dfea76b9..aebf315cb943a83ea87256cc4f4901cb691f67e9 100644 (file)
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -286,6 +286,7 @@ impl<'cfg> PackageRegistry<'cfg> {
 
     fn load(&mut self, source_id: &SourceId, kind: Kind) -> CargoResult<()> {
         (|| {
+            debug!("loading source {}", source_id);
             let source = self.source_config.load(source_id)?;
             assert_eq!(source.source_id(), source_id);
 

diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs
index 878def113912f4b7042d85b013b74c2ee4db319d..c5b134e4954867947b0cf81bc4f0b7ef1adb462f 100644 (file)
--- a/src/cargo/sources/registry/index.rs
+++ b/src/cargo/sources/registry/index.rs
@@ -75,6 +75,12 @@ impl<'cfg> RegistryIndex<'cfg> {
         name: &str,
         load: &mut RegistryData,
     ) -> CargoResult<Vec<(Summary, bool)>> {
+        // Prepare the `RegistryData` which will lazily initialize internal data
+        // structures. Note that this is also importantly needed to initialize
+        // to avoid deadlocks where we acquire a lock below but the `load`
+        // function inside *also* wants to acquire a lock. See an instance of
+        // this on #5551.
+        load.prepare()?;
         let (root, _lock) = if self.locked {
             let lock = self.path
                 .open_ro(Path::new(INDEX_LOCK), self.config, "the registry index");

diff --git a/src/cargo/sources/registry/local.rs b/src/cargo/sources/registry/local.rs
index fa97c42a2d3eed00f850e4d4d566896ba915fe88..32d745a88c3793fcdb6eef2561dd64e460ad48be 100644 (file)
--- a/src/cargo/sources/registry/local.rs
+++ b/src/cargo/sources/registry/local.rs
@@ -29,6 +29,10 @@ impl<'cfg> LocalRegistry<'cfg> {
 }
 
 impl<'cfg> RegistryData for LocalRegistry<'cfg> {
+    fn prepare(&self) -> CargoResult<()> {
+        Ok(())
+    }
+
     fn index_path(&self) -> &Filesystem {
         &self.index_path
     }

diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs
index 2e6f63228b9a94290429a7b936ff13c282c2ebae..2ea60e18f70f1bc491331ec0ed53801d7f065f79 100644 (file)
--- a/src/cargo/sources/registry/mod.rs
+++ b/src/cargo/sources/registry/mod.rs
@@ -295,6 +295,7 @@ impl<'a> RegistryDependency<'a> {
 }
 
 pub trait RegistryData {
+    fn prepare(&self) -> CargoResult<()>;
     fn index_path(&self) -> &Filesystem;
     fn load(
         &self,
@@ -427,6 +428,7 @@ impl<'cfg> Source for RegistrySource<'cfg> {
         // come back with no summaries, then our registry may need to be
         // updated, so we fall back to performing a lazy update.
         if dep.source_id().precise().is_some() && !self.updated {
+            debug!("attempting query without update");
             let mut called = false;
             self.index.query(dep, &mut *self.ops, &mut |s| {
                 called = true;
@@ -435,6 +437,7 @@ impl<'cfg> Source for RegistrySource<'cfg> {
             if called {
                 return Ok(());
             } else {
+                debug!("falling back to an update");
                 self.do_update()?;
             }
         }
@@ -464,6 +467,8 @@ impl<'cfg> Source for RegistrySource<'cfg> {
         // --precise` request
         if self.source_id.precise() != Some("locked") {
             self.do_update()?;
+        } else {
+            debug!("skipping update due to locked registry");
         }
         Ok(())
     }

diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs
index 3d50eda9c1344a0446e53b35c9f668e0adc83bba..dfd6ad6b019119ffb7c2e8e4c1cce64ed1da83b1 100644 (file)
--- a/src/cargo/sources/registry/remote.rs
+++ b/src/cargo/sources/registry/remote.rs
@@ -48,10 +48,12 @@ impl<'cfg> RemoteRegistry<'cfg> {
 
             // Fast path without a lock
             if let Ok(repo) = git2::Repository::open(&path) {
+                trace!("opened a repo without a lock");
                 return Ok(repo);
             }
 
             // Ok, now we need to lock and try the whole thing over again.
+            trace!("acquiring registry index lock");
             let lock =
                 self.index_path
                     .open_rw(Path::new(INDEX_LOCK), self.config, "the registry index")?;
@@ -71,7 +73,15 @@ impl<'cfg> RemoteRegistry<'cfg> {
                     // like enough time has passed or if we change the directory
                     // that the folder is located in, such as by changing the
                     // hash at the end of the directory.
-                    Ok(git2::Repository::init(&path)?)
+                    //
+                    // Note that in the meantime we also skip `init.templatedir`
+                    // as it can be misconfigured sometimes or otherwise add
+                    // things that we don't want.
+                    let mut opts = git2::RepositoryInitOptions::new();
+                    opts.external_template(false);
+                    Ok(git2::Repository::init_opts(&path, &opts).chain_err(|| {
+                        "failed to initialized index git repository"
+                    })?)
                 }
             }
         })
@@ -115,6 +125,11 @@ impl<'cfg> RemoteRegistry<'cfg> {
 }
 
 impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
+    fn prepare(&self) -> CargoResult<()> {
+        self.repo()?; // create intermediate dirs and initialize the repo
+        Ok(())
+    }
+
     fn index_path(&self) -> &Filesystem {
         &self.index_path
     }
@@ -140,7 +155,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
     }
 
     fn config(&mut self) -> CargoResult<Option<RegistryConfig>> {
-        self.repo()?; // create intermediate dirs and initialize the repo
+        debug!("loading config");
+        self.prepare()?;
         let _lock =
             self.index_path
                 .open_ro(Path::new(INDEX_LOCK), self.config, "the registry index")?;
@@ -149,6 +165,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
             config = Some(serde_json::from_slice(json)?);
             Ok(())
         })?;
+        trace!("config loaded");
         Ok(config)
     }
 
@@ -160,6 +177,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
             return Ok(());
         }
 
+        debug!("updating the index");
+
         // Ensure that we'll actually be able to acquire an HTTP handle later on
         // once we start trying to download crates. This will weed out any
         // problems with `.cargo/config` configuration related to HTTP.
@@ -168,7 +187,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
         // hit the index, which may not actually read this configuration.
         self.config.http()?;
 
-        self.repo()?;
+        self.prepare()?;
         self.head.set(None);
         *self.tree.borrow_mut() = None;
         let _lock =

diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs
index d3b1d43d02b113d9c62f914f47a24675863b153a..bbbaa785432c1e2008163f990d6ff0c2a44cc2c3 100644 (file)
--- a/src/cargo/util/errors.rs
+++ b/src/cargo/util/errors.rs
@@ -28,8 +28,11 @@ where
         D: fmt::Display + Send + Sync + 'static,
     {
         self.map_err(|failure| {
+            let err = failure.into();
             let context = f();
-            failure.into().context(context)
+            trace!("error: {}", err);
+            trace!("\tcontext: {}", context);
+            err.context(context)
         })
     }
 }

diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs
index 6121f3b8f02ff3548882d5ccbe83feffe365272e..4d632a7b925dc3e6f239d682e7eb3d118de66999 100644 (file)
--- a/tests/testsuite/registry.rs
+++ b/tests/testsuite/registry.rs
@@ -2,6 +2,7 @@ use std::fs::{self, File};
 use std::io::prelude::*;
 use std::path::PathBuf;
 
+use cargo::util::paths::remove_dir_all;
 use cargotest::cargo_process;
 use cargotest::support::git;
 use cargotest::support::paths::{self, CargoPathExt};
@@ -1800,3 +1801,48 @@ Caused by:
         ),
     );
 }
+
+#[test]
+fn git_init_templatedir_missing() {
+    Package::new("foo", "0.2.0").dep("bar", "*").publish();
+    Package::new("bar", "0.2.0").publish();
+
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+                [project]
+                name = "fo"
+                version = "0.5.0"
+                authors = []
+
+                [dependencies]
+                foo = "0.2"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    assert_that(
+        p.cargo("build"),
+        execs().with_status(0)
+    );
+
+    remove_dir_all(paths::home().join(".cargo/registry")).unwrap();
+    File::create(paths::home().join(".gitconfig"))
+        .unwrap()
+        .write_all(br#"
+            [init]
+            templatedir = nowhere
+        "#)
+        .unwrap();
+
+    assert_that(
+        p.cargo("build"),
+        execs().with_status(0)
+    );
+    assert_that(
+        p.cargo("build"),
+        execs().with_status(0)
+    );
+}