From 5cd68f87fec9ad8b54cc5b54862125551fc8ab38 Mon Sep 17 00:00:00 2001 From: Hugo van der Wijst Date: Tue, 22 Jan 2019 21:50:27 -0800 Subject: [PATCH 1/3] Fix race condition in local registry crate downloading. Copy crate and keep exclusive lock to it with local registries. Ensures that only one instance will try to extract the source of a new package. Fixes #6588. --- src/cargo/sources/registry/local.rs | 49 +++++++++++++++-------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/cargo/sources/registry/local.rs b/src/cargo/sources/registry/local.rs index ed649c9e559..ed8d86ef1e6 100644 --- a/src/cargo/sources/registry/local.rs +++ b/src/cargo/sources/registry/local.rs @@ -11,15 +11,15 @@ use hex; pub struct LocalRegistry<'cfg> { index_path: Filesystem, + cache_path: Filesystem, root: Filesystem, - src_path: Filesystem, config: &'cfg Config, } impl<'cfg> LocalRegistry<'cfg> { pub fn new(root: &Path, config: &'cfg Config, name: &str) -> LocalRegistry<'cfg> { LocalRegistry { - src_path: config.registry_source_path().join(name), + cache_path: config.registry_cache_path().join(name), index_path: Filesystem::new(root.join("index")), root: Filesystem::new(root.to_path_buf()), config, @@ -70,38 +70,39 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { } fn download(&mut self, pkg: PackageId, checksum: &str) -> CargoResult { - let crate_file = format!("{}-{}.crate", pkg.name(), pkg.version()); - let mut crate_file = self.root.open_ro(&crate_file, self.config, "crate file")?; - - // If we've already got an unpacked version of this crate, then skip the - // checksum below as it is in theory already verified. - let dst = format!("{}-{}", pkg.name(), pkg.version()); - if self.src_path.join(dst).into_path_unlocked().exists() { - return Ok(MaybeLock::Ready(crate_file)); + let filename = format!("{}-{}.crate", pkg.name(), pkg.version()); + + // Attempt to open an read-only lock first to avoid an exclusive write lock. + // + // If this fails then we fall through to the exclusive path where we copy + // the file. + if let Ok(dst) = self.cache_path.open_ro(&filename, self.config, &filename) { + let meta = dst.file().metadata()?; + if meta.len() > 0 { + return Ok(MaybeLock::Ready(dst)); + } } self.config.shell().status("Unpacking", pkg)?; - // We don't actually need to download anything per-se, we just need to - // verify the checksum matches the .crate file itself. + // Verify the checksum and copy over the .crate. + let mut buf = Vec::new(); + let mut crate_file_source = self.root.open_ro(&filename, self.config, "crate file")?; + let _ = crate_file_source + .read_to_end(&mut buf) + .chain_err(|| format!("failed to read `{}`", crate_file_source.path().display()))?; + let mut state = Sha256::new(); - let mut buf = [0; 64 * 1024]; - loop { - let n = crate_file - .read(&mut buf) - .chain_err(|| format!("failed to read `{}`", crate_file.path().display()))?; - if n == 0 { - break; - } - state.update(&buf[..n]); - } + state.update(&buf); if hex::encode(state.finish()) != checksum { failure::bail!("failed to verify the checksum of `{}`", pkg) } - crate_file.seek(SeekFrom::Start(0))?; + let mut dst = self.cache_path.open_rw(&filename, self.config, &filename)?; + dst.write_all(&buf)?; + dst.seek(SeekFrom::Start(0))?; - Ok(MaybeLock::Ready(crate_file)) + Ok(MaybeLock::Ready(dst)) } fn finish_download( From 1f87b6d35dbae147a6e58a44873c059c3ab1035d Mon Sep 17 00:00:00 2001 From: Hugo van der Wijst Date: Thu, 24 Jan 2019 08:19:43 -0800 Subject: [PATCH 2/3] Lock unpacking instead of local registry tarball. --- src/cargo/sources/registry/local.rs | 49 ++++++++++++++--------------- src/cargo/sources/registry/mod.rs | 40 +++++++++++++---------- 2 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/cargo/sources/registry/local.rs b/src/cargo/sources/registry/local.rs index ed8d86ef1e6..ed649c9e559 100644 --- a/src/cargo/sources/registry/local.rs +++ b/src/cargo/sources/registry/local.rs @@ -11,15 +11,15 @@ use hex; pub struct LocalRegistry<'cfg> { index_path: Filesystem, - cache_path: Filesystem, root: Filesystem, + src_path: Filesystem, config: &'cfg Config, } impl<'cfg> LocalRegistry<'cfg> { pub fn new(root: &Path, config: &'cfg Config, name: &str) -> LocalRegistry<'cfg> { LocalRegistry { - cache_path: config.registry_cache_path().join(name), + src_path: config.registry_source_path().join(name), index_path: Filesystem::new(root.join("index")), root: Filesystem::new(root.to_path_buf()), config, @@ -70,39 +70,38 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { } fn download(&mut self, pkg: PackageId, checksum: &str) -> CargoResult { - let filename = format!("{}-{}.crate", pkg.name(), pkg.version()); - - // Attempt to open an read-only lock first to avoid an exclusive write lock. - // - // If this fails then we fall through to the exclusive path where we copy - // the file. - if let Ok(dst) = self.cache_path.open_ro(&filename, self.config, &filename) { - let meta = dst.file().metadata()?; - if meta.len() > 0 { - return Ok(MaybeLock::Ready(dst)); - } + let crate_file = format!("{}-{}.crate", pkg.name(), pkg.version()); + let mut crate_file = self.root.open_ro(&crate_file, self.config, "crate file")?; + + // If we've already got an unpacked version of this crate, then skip the + // checksum below as it is in theory already verified. + let dst = format!("{}-{}", pkg.name(), pkg.version()); + if self.src_path.join(dst).into_path_unlocked().exists() { + return Ok(MaybeLock::Ready(crate_file)); } self.config.shell().status("Unpacking", pkg)?; - // Verify the checksum and copy over the .crate. - let mut buf = Vec::new(); - let mut crate_file_source = self.root.open_ro(&filename, self.config, "crate file")?; - let _ = crate_file_source - .read_to_end(&mut buf) - .chain_err(|| format!("failed to read `{}`", crate_file_source.path().display()))?; - + // We don't actually need to download anything per-se, we just need to + // verify the checksum matches the .crate file itself. let mut state = Sha256::new(); - state.update(&buf); + let mut buf = [0; 64 * 1024]; + loop { + let n = crate_file + .read(&mut buf) + .chain_err(|| format!("failed to read `{}`", crate_file.path().display()))?; + if n == 0 { + break; + } + state.update(&buf[..n]); + } if hex::encode(state.finish()) != checksum { failure::bail!("failed to verify the checksum of `{}`", pkg) } - let mut dst = self.cache_path.open_rw(&filename, self.config, &filename)?; - dst.write_all(&buf)?; - dst.seek(SeekFrom::Start(0))?; + crate_file.seek(SeekFrom::Start(0))?; - Ok(MaybeLock::Ready(dst)) + Ok(MaybeLock::Ready(crate_file)) } fn finish_download( diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index e2f756aeec0..d91e0db10e0 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -160,7 +160,7 @@ use std::borrow::Cow; use std::collections::BTreeMap; -use std::fs::File; +use std::io::Write; use std::path::{Path, PathBuf}; use flate2::read::GzDecoder; @@ -426,23 +426,28 @@ impl<'cfg> RegistrySource<'cfg> { /// /// No action is taken if the source looks like it's already unpacked. fn unpack_package(&self, pkg: PackageId, tarball: &FileLock) -> CargoResult { - let dst = self - .src_path - .join(&format!("{}-{}", pkg.name(), pkg.version())); - dst.create_dir()?; - // Note that we've already got the `tarball` locked above, and that - // implies a lock on the unpacked destination as well, so this access - // via `into_path_unlocked` should be ok. - let dst = dst.into_path_unlocked(); - let ok = dst.join(".cargo-ok"); - if ok.exists() { - return Ok(dst); + // The `.cargo-ok` file is used to track if the source is already + // unpacked and to lock the directory for unpacking. + let mut ok = { + let package_dir = format!("{}-{}", pkg.name(), pkg.version()); + let dst = self + .src_path + .join(&package_dir); + dst.create_dir()?; + dst.open_rw(".cargo-ok", self.config, &package_dir)? + }; + let unpack_dir = ok.parent().to_path_buf(); + + // If the file has any data, assume the source is already unpacked. + let meta = ok.file().metadata()?; + if meta.len() > 0 { + return Ok(unpack_dir); } let gz = GzDecoder::new(tarball.file()); let mut tar = Archive::new(gz); - let prefix = dst.file_name().unwrap(); - let parent = dst.parent().unwrap(); + let prefix = unpack_dir.file_name().unwrap(); + let parent = unpack_dir.parent().unwrap(); for entry in tar.entries()? { let mut entry = entry.chain_err(|| "failed to iterate over archive")?; let entry_path = entry @@ -470,8 +475,11 @@ impl<'cfg> RegistrySource<'cfg> { .unpack_in(parent) .chain_err(|| format!("failed to unpack entry at `{}`", entry_path.display()))?; } - File::create(&ok)?; - Ok(dst) + + // Write to the lock file to indicate that unpacking was successful. + write!(ok, "ok")?; + + Ok(unpack_dir) } fn do_update(&mut self) -> CargoResult<()> { From a23612dffd249c81e0de601a705cace84cb8b642 Mon Sep 17 00:00:00 2001 From: Hugo van der Wijst Date: Sun, 27 Jan 2019 19:47:28 -0800 Subject: [PATCH 3/3] First try opening the package source lock read-only. --- src/cargo/sources/registry/mod.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index d91e0db10e0..474e3a884a7 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -179,6 +179,7 @@ use crate::util::to_url::ToUrl; use crate::util::{internal, CargoResult, Config, FileLock, Filesystem}; const INDEX_LOCK: &str = ".cargo-index-lock"; +const PACKAGE_SOURCE_LOCK: &str = ".cargo-ok"; pub const CRATES_IO_INDEX: &str = "https://github.com/rust-lang/crates.io-index"; pub const CRATES_IO_REGISTRY: &str = "crates-io"; const CRATE_TEMPLATE: &str = "{crate}"; @@ -434,7 +435,19 @@ impl<'cfg> RegistrySource<'cfg> { .src_path .join(&package_dir); dst.create_dir()?; - dst.open_rw(".cargo-ok", self.config, &package_dir)? + + // Attempt to open a read-only copy first to avoid an exclusive write + // lock and also work with read-only filesystems. If the file has + // any data, assume the source is already unpacked. + if let Ok(ok) = dst.open_ro(PACKAGE_SOURCE_LOCK, self.config, &package_dir) { + let meta = ok.file().metadata()?; + if meta.len() > 0 { + let unpack_dir = ok.parent().to_path_buf(); + return Ok(unpack_dir); + } + } + + dst.open_rw(PACKAGE_SOURCE_LOCK, self.config, &package_dir)? }; let unpack_dir = ok.parent().to_path_buf();