From: Alex Crichton Date: Sat, 26 May 2018 02:12:53 +0000 (-0700) Subject: Fix an issue of deadlock in Cargo X-Git-Tag: archive/raspbian/0.35.0-2+rpi1~3^2^2^2^2^2^2^2~22^2^2~17^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=1868998bec6a311d2dd8e4549968df58d1b787ab;p=cargo.git 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 904179c7a..aebf315cb 100644 --- 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 878def113..c5b134e49 100644 --- 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> { + // 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 fa97c42a2..32d745a88 100644 --- 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 2e6f63228..2ea60e18f 100644 --- 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 3d50eda9c..dfd6ad6b0 100644 --- 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> { - 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 d3b1d43d0..bbbaa7854 100644 --- 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 6121f3b8f..4d632a7b9 100644 --- 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) + ); +}