Skip to content

Commit

Permalink
fix(verify-source): Detect published-at = 0x0
Browse files Browse the repository at this point in the history
## Description

Detect when the `published-at` field in `Move.toml` or `Move.lock` has
been explicitly set to `0x0` and treat that as if it was not set at all.

This is not commonly done by people, but it happens in our test set-up.

This also required converting the field into an `ObjectID` earlier in
the pipeline, which introduced some further changes in the codebase.

## Test plan

```
sui-source-validation$ cargo nextest run
```
  • Loading branch information
amnn committed Aug 14, 2024
1 parent d23cdfa commit 97cf57e
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 33 deletions.
2 changes: 1 addition & 1 deletion crates/sui-move-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ pub struct PackageDependencies {
pub invalid: BTreeMap<Symbol, String>,
/// Set of dependencies that have conflicting `published-at` addresses. The key refers to
/// the package, and the tuple refers to the address in the (Move.lock, Move.toml) respectively.
pub conflicting: BTreeMap<Symbol, (String, String)>,
pub conflicting: BTreeMap<Symbol, (ObjectID, ObjectID)>,
}

/// Partition packages in `resolution_graph` into one of four groups:
Expand Down
91 changes: 60 additions & 31 deletions crates/sui-package-management/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use anyhow::{bail, Context};
use std::collections::HashMap;
use std::fs::File;
use std::path::{Path, PathBuf};
use std::str::FromStr;
Expand Down Expand Up @@ -29,8 +30,8 @@ pub enum PublishedAtError {
Invalid(String),
NotPresent,
Conflict {
id_lock: String,
id_manifest: String,
id_lock: ObjectID,
id_manifest: ObjectID,
},
}

Expand Down Expand Up @@ -140,45 +141,42 @@ pub fn resolve_published_id(
) -> Result<ObjectID, PublishedAtError> {
// Look up a valid `published-at` in the `Move.toml` first, which we'll
// return if the Move.lock does not manage addresses.
let published_id_in_manifest = match published_at_property(package) {
Ok(v) => Some(v),
Err(PublishedAtError::NotPresent) => None,
Err(e) => return Err(e), // An existing but invalid `published-at` in `Move.toml` should fail early.
};
let published_id_in_manifest = manifest_published_at(package);

match published_id_in_manifest {
Ok(_) | Err(PublishedAtError::NotPresent) => { /* nop */ }
Err(e) => {
return Err(e);
}
}

let lock = package.package_path.join(SourcePackageLayout::Lock.path());
let Ok(mut lock_file) = File::open(lock.clone()) else {
return match published_id_in_manifest {
Some(v) => {
ObjectID::from_str(v.as_str()).map_err(|_| PublishedAtError::Invalid(v.to_owned()))
}
None => Err(PublishedAtError::NotPresent),
};
return published_id_in_manifest;
};
let managed_packages = ManagedPackage::read(&mut lock_file).ok();

// Find the environment and ManagedPackage data for this chain_id.
let id_in_lock_for_chain_id = managed_packages.and_then(|m| {
let chain_id = chain_id.as_ref()?;
m.into_iter()
.find_map(|(_, v)| (v.chain_id == *chain_id).then_some(v.latest_published_id))
});

let package_id = match (id_in_lock_for_chain_id, published_id_in_manifest) {
(Some(id_lock), Some(id_manifest)) if id_lock != id_manifest => {
return Err(PublishedAtError::Conflict {
let id_in_lock_for_chain_id =
lock_published_at(ManagedPackage::read(&mut lock_file).ok(), chain_id.as_ref());

match (id_in_lock_for_chain_id, published_id_in_manifest) {
(Ok(id_lock), Ok(id_manifest)) if id_lock != id_manifest => {
Err(PublishedAtError::Conflict {
id_lock,
id_manifest,
})
}
(Some(id_lock), _) => id_lock,
(None, Some(id_manifest)) => id_manifest, /* No info in Move.lock: Fall back to manifest */
_ => return Err(PublishedAtError::NotPresent), /* Neither in Move.toml nor Move.lock */
};
ObjectID::from_str(package_id.as_str())
.map_err(|_| PublishedAtError::Invalid(package_id.to_owned()))

(Ok(id), _) | (_, Ok(id)) => Ok(id),

// We return early (above) if we failed to read the ID from the manifest for some reason
// other than it not being present, so at this point, we can defer to whatever error came
// from the lock file (Ok case is handled above).
(from_lock, Err(_)) => from_lock,
}
}

fn published_at_property(package: &Package) -> Result<String, PublishedAtError> {
fn manifest_published_at(package: &Package) -> Result<ObjectID, PublishedAtError> {
let Some(value) = package
.source_package
.package
Expand All @@ -187,5 +185,36 @@ fn published_at_property(package: &Package) -> Result<String, PublishedAtError>
else {
return Err(PublishedAtError::NotPresent);
};
Ok(value.to_string())

let id =
ObjectID::from_str(value.as_str()).map_err(|_| PublishedAtError::Invalid(value.clone()))?;

if id == ObjectID::ZERO {
Err(PublishedAtError::NotPresent)
} else {
Ok(id)
}
}

fn lock_published_at(
lock: Option<HashMap<String, ManagedPackage>>,
chain_id: Option<&String>,
) -> Result<ObjectID, PublishedAtError> {
let (Some(lock), Some(chain_id)) = (lock, chain_id) else {
return Err(PublishedAtError::NotPresent);
};

let managed_package = lock
.into_values()
.find(|v| v.chain_id == *chain_id)
.ok_or(PublishedAtError::NotPresent)?;

let id = ObjectID::from_str(managed_package.latest_published_id.as_str())
.map_err(|_| PublishedAtError::Invalid(managed_package.latest_published_id.clone()))?;

if id == ObjectID::ZERO {
Err(PublishedAtError::NotPresent)
} else {
Ok(id)
}
}
2 changes: 1 addition & 1 deletion crates/sui/tests/cli_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2080,7 +2080,7 @@ async fn test_package_management_on_upgrade_command_conflict() -> Result<(), any
let err_string = err_string.replace(&package.object_id().to_string(), "<elided-for-test>");

let expect = expect![[r#"
Conflicting published package address: `Move.toml` contains published-at address 0xbad but `Move.lock` file contains published-at address <elided-for-test>. You may want to:
Conflicting published package address: `Move.toml` contains published-at address 0x0000000000000000000000000000000000000000000000000000000000000bad but `Move.lock` file contains published-at address <elided-for-test>. You may want to:
- delete the published-at address in the `Move.toml` if the `Move.lock` address is correct; OR
- update the `Move.lock` address using the `sui manage-package` command to be the same as the `Move.toml`; OR
Expand Down

0 comments on commit 97cf57e

Please sign in to comment.