Skip to content

Commit c60c065

Browse files
committed
fix: clear cache for old .cargo-ok format
In 1.71, `.cargo-ok` changed to contain a JSON `{ v: 1 }` to indicate the version of it. A failure of parsing will result in a heavy-hammer approach that unpacks the `.crate` file again. This is in response to a security issue that the unpacking didn't respect umask on Unix systems.
1 parent 4fafa69 commit c60c065

File tree

2 files changed

+108
-12
lines changed

2 files changed

+108
-12
lines changed

src/cargo/sources/registry/mod.rs

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@
184184
//! [`IndexPackage`]: index::IndexPackage
185185
186186
use std::collections::HashSet;
187+
use std::fs;
187188
use std::fs::{File, OpenOptions};
188189
use std::io;
189190
use std::io::Read;
@@ -196,6 +197,7 @@ use cargo_util::paths::{self, exclude_from_backups_and_indexing};
196197
use flate2::read::GzDecoder;
197198
use log::debug;
198199
use serde::Deserialize;
200+
use serde::Serialize;
199201
use tar::Archive;
200202

201203
use crate::core::dependency::Dependency;
@@ -219,6 +221,14 @@ pub const CRATES_IO_HTTP_INDEX: &str = "sparse+https://index.crates.io/";
219221
pub const CRATES_IO_REGISTRY: &str = "crates-io";
220222
pub const CRATES_IO_DOMAIN: &str = "crates.io";
221223

224+
/// The content inside `.cargo-ok`.
225+
/// See [`RegistrySource::unpack_package`] for more.
226+
#[derive(Deserialize, Serialize)]
227+
struct LockMetadata {
228+
/// The version of `.cargo-ok` file
229+
v: u32,
230+
}
231+
222232
/// A [`Source`] implementation for a local or a remote registry.
223233
///
224234
/// This contains common functionality that is shared between each registry
@@ -546,6 +556,11 @@ impl<'cfg> RegistrySource<'cfg> {
546556
/// `.crate` files making `.cargo-ok` a symlink causing cargo to write "ok"
547557
/// to any arbitrary file on the filesystem it has permission to.
548558
///
559+
/// In 1.71, `.cargo-ok` changed to contain a JSON `{ v: 1 }` to indicate
560+
/// the version of it. A failure of parsing will result in a heavy-hammer
561+
/// approach that unpacks the `.crate` file again. This is in response to a
562+
/// security issue that the unpacking didn't respect umask on Unix systems.
563+
///
549564
/// This is all a long-winded way of explaining the circumstances that might
550565
/// cause a directory to contain a `.cargo-ok` file that is empty or
551566
/// otherwise corrupted. Either this was extracted by a version of Rust
@@ -567,15 +582,23 @@ impl<'cfg> RegistrySource<'cfg> {
567582
let path = dst.join(PACKAGE_SOURCE_LOCK);
568583
let path = self.config.assert_package_cache_locked(&path);
569584
let unpack_dir = path.parent().unwrap();
570-
match path.metadata() {
571-
Ok(meta) if meta.len() > 0 => return Ok(unpack_dir.to_path_buf()),
572-
Ok(_meta) => {
573-
// See comment of `unpack_package` about why removing all stuff.
574-
log::warn!("unexpected length of {path:?}, clearing cache");
575-
paths::remove_dir_all(dst.as_path_unlocked())?;
576-
}
585+
match fs::read_to_string(path) {
586+
Ok(ok) => match serde_json::from_str::<LockMetadata>(&ok) {
587+
Ok(lock_meta) if lock_meta.v == 1 => {
588+
return Ok(unpack_dir.to_path_buf());
589+
}
590+
_ => {
591+
if ok == "ok" {
592+
log::debug!("old `ok` content found, clearing cache");
593+
} else {
594+
log::warn!("unrecognized .cargo-ok content, clearing cache: {ok}");
595+
}
596+
// See comment of `unpack_package` about why removing all stuff.
597+
paths::remove_dir_all(dst.as_path_unlocked())?;
598+
}
599+
},
577600
Err(e) if e.kind() == io::ErrorKind::NotFound => {}
578-
Err(e) => anyhow::bail!("failed to access package completion {path:?}: {e}"),
601+
Err(e) => anyhow::bail!("unable to read .cargo-ok file at {path:?}: {e}"),
579602
}
580603
dst.create_dir()?;
581604
let mut tar = {
@@ -639,7 +662,9 @@ impl<'cfg> RegistrySource<'cfg> {
639662
.write(true)
640663
.open(&path)
641664
.with_context(|| format!("failed to open `{}`", path.display()))?;
642-
write!(ok, "ok")?;
665+
666+
let lock_meta = LockMetadata { v: 1 };
667+
write!(ok, "{}", serde_json::to_string(&lock_meta).unwrap())?;
643668

644669
Ok(unpack_dir.to_path_buf())
645670
}

tests/testsuite/registry.rs

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2546,7 +2546,7 @@ fn package_lock_inside_package_is_overwritten() {
25462546
.join("bar-0.0.1")
25472547
.join(".cargo-ok");
25482548

2549-
assert_eq!(ok.metadata().unwrap().len(), 2);
2549+
assert_eq!(ok.metadata().unwrap().len(), 7);
25502550
}
25512551

25522552
#[cargo_test]
@@ -2586,7 +2586,7 @@ fn package_lock_as_a_symlink_inside_package_is_overwritten() {
25862586
let librs = pkg_root.join("src/lib.rs");
25872587

25882588
// Is correctly overwritten and doesn't affect the file linked to
2589-
assert_eq!(ok.metadata().unwrap().len(), 2);
2589+
assert_eq!(ok.metadata().unwrap().len(), 7);
25902590
assert_eq!(fs::read_to_string(librs).unwrap(), "pub fn f() {}");
25912591
}
25922592

@@ -3135,7 +3135,7 @@ fn corrupted_ok_overwritten() {
31353135
fs::write(&ok, "").unwrap();
31363136
assert_eq!(fs::read_to_string(&ok).unwrap(), "");
31373137
p.cargo("fetch").with_stderr("").run();
3138-
assert_eq!(fs::read_to_string(&ok).unwrap(), "ok");
3138+
assert_eq!(fs::read_to_string(&ok).unwrap(), r#"{"v":1}"#);
31393139
}
31403140

31413141
#[cargo_test]
@@ -3458,3 +3458,74 @@ fn set_mask_during_unpacking() {
34583458
let metadata = fs::metadata(src_file_path("example.sh")).unwrap();
34593459
assert_eq!(metadata.mode() & 0o777, 0o777 & !umask);
34603460
}
3461+
3462+
#[cargo_test]
3463+
fn unpack_again_when_cargo_ok_is_unrecognized() {
3464+
Package::new("bar", "1.0.0").publish();
3465+
3466+
let p = project()
3467+
.file(
3468+
"Cargo.toml",
3469+
r#"
3470+
[package]
3471+
name = "foo"
3472+
version = "0.1.0"
3473+
3474+
[dependencies]
3475+
bar = "1.0"
3476+
"#,
3477+
)
3478+
.file("src/lib.rs", "")
3479+
.build();
3480+
3481+
p.cargo("fetch")
3482+
.with_stderr(
3483+
"\
3484+
[UPDATING] `dummy-registry` index
3485+
[DOWNLOADING] crates ...
3486+
[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`)
3487+
",
3488+
)
3489+
.run();
3490+
3491+
let src_file_path = |path: &str| {
3492+
glob::glob(
3493+
paths::home()
3494+
.join(".cargo/registry/src/*/bar-1.0.0/")
3495+
.join(path)
3496+
.to_str()
3497+
.unwrap(),
3498+
)
3499+
.unwrap()
3500+
.next()
3501+
.unwrap()
3502+
.unwrap()
3503+
};
3504+
3505+
// Change permissions to simulate the old behavior not respecting umask.
3506+
let lib_rs = src_file_path("src/lib.rs");
3507+
let cargo_ok = src_file_path(".cargo-ok");
3508+
let mut perms = fs::metadata(&lib_rs).unwrap().permissions();
3509+
assert!(!perms.readonly());
3510+
perms.set_readonly(true);
3511+
fs::set_permissions(&lib_rs, perms).unwrap();
3512+
let ok = fs::read_to_string(&cargo_ok).unwrap();
3513+
assert_eq!(&ok, r#"{"v":1}"#);
3514+
3515+
p.cargo("fetch").with_stderr("").run();
3516+
3517+
// Without changing `.cargo-ok`, a unpack won't be triggered.
3518+
let perms = fs::metadata(&lib_rs).unwrap().permissions();
3519+
assert!(perms.readonly());
3520+
3521+
// Write "ok" to simulate the old behavior and trigger the unpack again.
3522+
fs::write(&cargo_ok, "ok").unwrap();
3523+
3524+
p.cargo("fetch").with_stderr("").run();
3525+
3526+
// Permission has been restored and `.cargo-ok` is in the new format.
3527+
let perms = fs::metadata(lib_rs).unwrap().permissions();
3528+
assert!(!perms.readonly());
3529+
let ok = fs::read_to_string(&cargo_ok).unwrap();
3530+
assert_eq!(&ok, r#"{"v":1}"#);
3531+
}

0 commit comments

Comments
 (0)