Skip to content

Commit

Permalink
fix(resolver): Report invalid index entries (#14927)
Browse files Browse the repository at this point in the history
### What does this PR try to resolve?

While #14897 reported packages with an unsupported index schema version,
that only worked if the changes in the schema version did not cause
errors in deserializing `IndexPackage` or in generating a `Summary`.

This extends that change by recoverying on error with a more lax,
incomplete parse of `IndexPackage` which should always generate a valid
`Summary`.

To help with a buggy Index, we also will report as many as we can.
This does not provide a way to report to users or log on cache reads if
the index entry is not at least `{"name": "<string>", "vers":
"<semver>"}`.

Fixes #10623
Fixes #14894

### How should we test and review this PR?

My biggest paranoia is some bad interaction with the index cache
including more "invalid" index entries.
That should be ok as we will ignore the invalid entry in the cache when
loading it.
Ignoring of invalid entries dates back to #6880 when the index cache was
introduced.

### Additional information
  • Loading branch information
weihanglo authored Dec 13, 2024
2 parents 63ecc8d + 1d54a2b commit 876ea2c
Show file tree
Hide file tree
Showing 8 changed files with 250 additions and 57 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ cargo-credential-macos-keychain = { version = "0.4.7", path = "credential/cargo-
cargo-credential-wincred = { version = "0.4.7", path = "credential/cargo-credential-wincred" }
cargo-platform = { path = "crates/cargo-platform", version = "0.2.0" }
cargo-test-macro = { version = "0.4.0", path = "crates/cargo-test-macro" }
cargo-test-support = { version = "0.6.0", path = "crates/cargo-test-support" }
cargo-test-support = { version = "0.7.0", path = "crates/cargo-test-support" }
cargo-util = { version = "0.2.14", path = "crates/cargo-util" }
cargo-util-schemas = { version = "0.7.0", path = "crates/cargo-util-schemas" }
cargo_metadata = "0.19.0"
Expand Down
2 changes: 1 addition & 1 deletion crates/cargo-test-support/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "cargo-test-support"
version = "0.6.1"
version = "0.7.0"
edition.workspace = true
rust-version = "1.83" # MSRV:1
license.workspace = true
Expand Down
50 changes: 32 additions & 18 deletions crates/cargo-test-support/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,8 @@ pub struct Package {
features: FeatureMap,
local: bool,
alternative: bool,
invalid_json: bool,
invalid_index_line: bool,
index_line: Option<String>,
edition: Option<String>,
resolver: Option<String>,
proc_macro: bool,
Expand Down Expand Up @@ -1251,7 +1252,8 @@ impl Package {
features: BTreeMap::new(),
local: false,
alternative: false,
invalid_json: false,
invalid_index_line: false,
index_line: None,
edition: None,
resolver: None,
proc_macro: false,
Expand Down Expand Up @@ -1422,8 +1424,16 @@ impl Package {

/// Causes the JSON line emitted in the index to be invalid, presumably
/// causing Cargo to skip over this version.
pub fn invalid_json(&mut self, invalid: bool) -> &mut Package {
self.invalid_json = invalid;
pub fn invalid_index_line(&mut self, invalid: bool) -> &mut Package {
self.invalid_index_line = invalid;
self
}

/// Override the auto-generated index line
///
/// This can give more control over error cases than [`Package::invalid_index_line`]
pub fn index_line(&mut self, line: &str) -> &mut Package {
self.index_line = Some(line.to_owned());
self
}

Expand Down Expand Up @@ -1496,22 +1506,26 @@ impl Package {
let c = t!(fs::read(&self.archive_dst()));
cksum(&c)
};
let name = if self.invalid_json {
serde_json::json!(1)
let line = if let Some(line) = self.index_line.clone() {
line
} else {
serde_json::json!(self.name)
let name = if self.invalid_index_line {
serde_json::json!(1)
} else {
serde_json::json!(self.name)
};
create_index_line(
name,
&self.vers,
deps,
&cksum,
self.features.clone(),
self.yanked,
self.links.clone(),
self.rust_version.as_deref(),
self.v,
)
};
let line = create_index_line(
name,
&self.vers,
deps,
&cksum,
self.features.clone(),
self.yanked,
self.links.clone(),
self.rust_version.as_deref(),
self.v,
);

let registry_path = if self.alternative {
alt_registry_path()
Expand Down
7 changes: 7 additions & 0 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,13 @@ pub(super) fn activation_error(
);
}
}
IndexSummary::Invalid(summary) => {
let _ = writeln!(
&mut msg,
" version {}'s index entry is invalid",
summary.version()
);
}
}
}
} else if let Some(candidates) = alt_versions(registry, dep) {
Expand Down
129 changes: 94 additions & 35 deletions src/cargo/sources/registry/index/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ pub enum IndexSummary {
Offline(Summary),
/// From a newer schema version and is likely incomplete or inaccurate
Unsupported(Summary, u32),
/// An error was encountered despite being a supported schema version
Invalid(Summary),
}

impl IndexSummary {
Expand All @@ -144,7 +146,8 @@ impl IndexSummary {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum,
| IndexSummary::Unsupported(sum, _)
| IndexSummary::Invalid(sum) => sum,
}
}

Expand All @@ -154,7 +157,8 @@ impl IndexSummary {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum,
| IndexSummary::Unsupported(sum, _)
| IndexSummary::Invalid(sum) => sum,
}
}

Expand All @@ -164,17 +168,13 @@ impl IndexSummary {
IndexSummary::Yanked(s) => IndexSummary::Yanked(f(s)),
IndexSummary::Offline(s) => IndexSummary::Offline(f(s)),
IndexSummary::Unsupported(s, v) => IndexSummary::Unsupported(f(s), v.clone()),
IndexSummary::Invalid(s) => IndexSummary::Invalid(f(s)),
}
}

/// Extract the package id from any variant
pub fn package_id(&self) -> PackageId {
match self {
IndexSummary::Candidate(sum)
| IndexSummary::Yanked(sum)
| IndexSummary::Offline(sum)
| IndexSummary::Unsupported(sum, _) => sum.package_id(),
}
self.as_summary().package_id()
}

/// Returns `true` if the index summary is [`Yanked`].
Expand Down Expand Up @@ -259,8 +259,52 @@ pub struct IndexPackage<'a> {
pub v: Option<u32>,
}

/// A dependency as encoded in the [`IndexPackage`] index JSON.
impl IndexPackage<'_> {
fn to_summary(&self, source_id: SourceId) -> CargoResult<Summary> {
// ****CAUTION**** Please be extremely careful with returning errors, see
// `IndexSummary::parse` for details
let pkgid = PackageId::new(self.name.into(), self.vers.clone(), source_id);
let deps = self
.deps
.iter()
.map(|dep| dep.clone().into_dep(source_id))
.collect::<CargoResult<Vec<_>>>()?;
let mut features = self.features.clone();
if let Some(features2) = &self.features2 {
for (name, values) in features2 {
features.entry(*name).or_default().extend(values);
}
}
let mut summary = Summary::new(
pkgid,
deps,
&features,
self.links,
self.rust_version.clone(),
)?;
summary.set_checksum(self.cksum.clone());
Ok(summary)
}
}

#[derive(Deserialize, Serialize)]
struct IndexPackageMinimum {
name: InternedString,
vers: Version,
}

#[derive(Deserialize, Serialize, Default)]
struct IndexPackageRustVersion {
rust_version: Option<RustVersion>,
}

#[derive(Deserialize, Serialize, Default)]
struct IndexPackageV {
v: Option<u32>,
}

/// A dependency as encoded in the [`IndexPackage`] index JSON.
#[derive(Deserialize, Serialize, Clone)]
pub struct RegistryDependency<'a> {
/// Name of the dependency. If the dependency is renamed, the original
/// would be stored in [`RegistryDependency::package`].
Expand Down Expand Up @@ -706,32 +750,45 @@ impl IndexSummary {
// between different versions that understand the index differently.
// Make sure to consider the INDEX_V_MAX and CURRENT_CACHE_VERSION
// values carefully when making changes here.
let IndexPackage {
name,
vers,
cksum,
deps,
mut features,
features2,
yanked,
links,
rust_version,
v,
} = serde_json::from_slice(line)?;
let v = v.unwrap_or(1);
tracing::trace!("json parsed registry {}/{}", name, vers);
let pkgid = PackageId::new(name.into(), vers.clone(), source_id);
let deps = deps
.into_iter()
.map(|dep| dep.into_dep(source_id))
.collect::<CargoResult<Vec<_>>>()?;
if let Some(features2) = features2 {
for (name, values) in features2 {
features.entry(name).or_default().extend(values);
let index_summary = (|| {
let index = serde_json::from_slice::<IndexPackage<'_>>(line)?;
let summary = index.to_summary(source_id)?;
Ok((index, summary))
})();
let (index, summary, valid) = match index_summary {
Ok((index, summary)) => (index, summary, true),
Err(err) => {
let Ok(IndexPackageMinimum { name, vers }) =
serde_json::from_slice::<IndexPackageMinimum>(line)
else {
// If we can't recover, prefer the original error
return Err(err);
};
tracing::info!(
"recoverying from failed parse of registry package {name}@{vers}: {err}"
);
let IndexPackageRustVersion { rust_version } =
serde_json::from_slice::<IndexPackageRustVersion>(line).unwrap_or_default();
let IndexPackageV { v } =
serde_json::from_slice::<IndexPackageV>(line).unwrap_or_default();
let index = IndexPackage {
name,
vers,
rust_version,
v,
deps: Default::default(),
features: Default::default(),
features2: Default::default(),
cksum: Default::default(),
yanked: Default::default(),
links: Default::default(),
};
let summary = index.to_summary(source_id)?;
(index, summary, false)
}
}
let mut summary = Summary::new(pkgid, deps, &features, links, rust_version)?;
summary.set_checksum(cksum);
};
let v = index.v.unwrap_or(1);
tracing::trace!("json parsed registry {}/{}", index.name, index.vers);

let v_max = if bindeps {
INDEX_V_MAX + 1
Expand All @@ -741,7 +798,9 @@ impl IndexSummary {

if v_max < v {
Ok(IndexSummary::Unsupported(summary, v))
} else if yanked.unwrap_or(false) {
} else if !valid {
Ok(IndexSummary::Invalid(summary))
} else if index.yanked.unwrap_or(false) {
Ok(IndexSummary::Yanked(summary))
} else {
Ok(IndexSummary::Candidate(summary))
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,9 @@ impl<'gctx> Source for RegistrySource<'gctx> {
summary.version()
);
}
IndexSummary::Invalid(summary) => {
tracing::debug!("invalid ({} {})", summary.name(), summary.version());
}
IndexSummary::Offline(summary) => {
tracing::debug!("offline ({} {})", summary.name(), summary.version());
}
Expand Down
Loading

0 comments on commit 876ea2c

Please sign in to comment.