From c7f0c6186ad376f97fe95601a41bc841c90ba430 Mon Sep 17 00:00:00 2001 From: Rijnard van Tonder Date: Wed, 1 Feb 2023 10:29:33 -0600 Subject: [PATCH] source validation: emit multiple errors (#7971) --- .../sui-source-validation/fixture/c/Move.toml | 3 + .../fixture/c/sources/b.move | 8 +++ .../fixture/c/sources/c.move | 8 +++ .../fixture/c/sources/d.move | 8 +++ .../sui-source-validation/fixture/d/Move.toml | 7 +++ .../fixture/d/sources/a.move | 13 ++++ crates/sui-source-validation/src/lib.rs | 52 +++++++++++++--- crates/sui-source-validation/src/tests.rs | 60 +++++++++++++++++++ 8 files changed, 150 insertions(+), 9 deletions(-) create mode 100644 crates/sui-source-validation/fixture/c/Move.toml create mode 100644 crates/sui-source-validation/fixture/c/sources/b.move create mode 100644 crates/sui-source-validation/fixture/c/sources/c.move create mode 100644 crates/sui-source-validation/fixture/c/sources/d.move create mode 100644 crates/sui-source-validation/fixture/d/Move.toml create mode 100644 crates/sui-source-validation/fixture/d/sources/a.move diff --git a/crates/sui-source-validation/fixture/c/Move.toml b/crates/sui-source-validation/fixture/c/Move.toml new file mode 100644 index 0000000000000..d07ae5b6848f7 --- /dev/null +++ b/crates/sui-source-validation/fixture/c/Move.toml @@ -0,0 +1,3 @@ +[package] +name = "c" +version = "0.0.1" diff --git a/crates/sui-source-validation/fixture/c/sources/b.move b/crates/sui-source-validation/fixture/c/sources/b.move new file mode 100644 index 0000000000000..9af2b2dd2a2c2 --- /dev/null +++ b/crates/sui-source-validation/fixture/c/sources/b.move @@ -0,0 +1,8 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +module c::b { + public fun b(): u64 { + 42 + } +} diff --git a/crates/sui-source-validation/fixture/c/sources/c.move b/crates/sui-source-validation/fixture/c/sources/c.move new file mode 100644 index 0000000000000..03f9c506f19ac --- /dev/null +++ b/crates/sui-source-validation/fixture/c/sources/c.move @@ -0,0 +1,8 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +module c::c { + public fun c(): u64 { + 43 + } +} diff --git a/crates/sui-source-validation/fixture/c/sources/d.move b/crates/sui-source-validation/fixture/c/sources/d.move new file mode 100644 index 0000000000000..830f9c0d6a023 --- /dev/null +++ b/crates/sui-source-validation/fixture/c/sources/d.move @@ -0,0 +1,8 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +module c::d { + public fun d(): u64 { + 44 + } +} diff --git a/crates/sui-source-validation/fixture/d/Move.toml b/crates/sui-source-validation/fixture/d/Move.toml new file mode 100644 index 0000000000000..c13dd141d244f --- /dev/null +++ b/crates/sui-source-validation/fixture/d/Move.toml @@ -0,0 +1,7 @@ +[package] +name = "d" +version = "0.0.1" + +[dependencies] +b = { local = "../b" } +c = { local = "../c" } diff --git a/crates/sui-source-validation/fixture/d/sources/a.move b/crates/sui-source-validation/fixture/d/sources/a.move new file mode 100644 index 0000000000000..0b15099d2d968 --- /dev/null +++ b/crates/sui-source-validation/fixture/d/sources/a.move @@ -0,0 +1,13 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +module d::d { + use b::b::b; + use c::c::c; + + public fun d(): u64 { + let var = 123; + let _ = var + 4; + b() + c() + } +} diff --git a/crates/sui-source-validation/src/lib.rs b/crates/sui-source-validation/src/lib.rs index 6873c58a95507..15b25275627ba 100644 --- a/crates/sui-source-validation/src/lib.rs +++ b/crates/sui-source-validation/src/lib.rs @@ -1,6 +1,7 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 +use core::fmt; use futures::future; use move_binary_format::access::ModuleAccess; use move_binary_format::CompiledModule; @@ -56,6 +57,33 @@ pub enum SourceVerificationError { InvalidModuleFailure { name: String, message: String }, } +#[derive(Debug, Error)] +pub struct AggregateSourceVerificationError(Vec); + +impl From for AggregateSourceVerificationError { + fn from(error: SourceVerificationError) -> Self { + AggregateSourceVerificationError(vec![error]) + } +} + +impl fmt::Display for AggregateSourceVerificationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let AggregateSourceVerificationError(errors) = self; + match &errors[..] { + [] => unreachable!("Aggregate error with no errors"), + [error] => write!(f, "{}", error)?, + errors => { + writeln!(f, "Multiple source verification errors found:")?; + for error in errors { + write!(f, "\n- {}", error)?; + } + return Ok(()); + } + }; + Ok(()) + } +} + /// How to handle package source during bytecode verification. #[derive(PartialEq, Eq)] pub enum SourceMode { @@ -94,7 +122,7 @@ impl<'a> BytecodeSourceVerifier<'a> { &self, compiled_package: &CompiledPackage, root_on_chain_address: AccountAddress, - ) -> Result<(), SourceVerificationError> { + ) -> Result<(), AggregateSourceVerificationError> { self.verify_package( compiled_package, /* verify_deps */ true, @@ -109,7 +137,7 @@ impl<'a> BytecodeSourceVerifier<'a> { &self, compiled_package: &CompiledPackage, root_on_chain_address: AccountAddress, - ) -> Result<(), SourceVerificationError> { + ) -> Result<(), AggregateSourceVerificationError> { self.verify_package( compiled_package, /* verify_deps */ false, @@ -123,7 +151,7 @@ impl<'a> BytecodeSourceVerifier<'a> { pub async fn verify_package_deps( &self, compiled_package: &CompiledPackage, - ) -> Result<(), SourceVerificationError> { + ) -> Result<(), AggregateSourceVerificationError> { self.verify_package( compiled_package, /* verify_deps */ true, @@ -141,11 +169,11 @@ impl<'a> BytecodeSourceVerifier<'a> { compiled_package: &CompiledPackage, verify_deps: bool, source_mode: SourceMode, - ) -> Result<(), SourceVerificationError> { + ) -> Result<(), AggregateSourceVerificationError> { // On-chain address for matching root package cannot be zero if let SourceMode::VerifyAt(root_address) = &source_mode { if *root_address == AccountAddress::ZERO { - return Err(SourceVerificationError::ZeroOnChainAddresSpecifiedFailure); + return Err(SourceVerificationError::ZeroOnChainAddresSpecifiedFailure.into()); } } @@ -154,17 +182,19 @@ impl<'a> BytecodeSourceVerifier<'a> { .on_chain_bytes(local_modules.keys().map(|(addr, _)| *addr)) .await?; + let mut errors = Vec::new(); for ((address, module), (package, local_bytes)) in local_modules { let Some(on_chain_bytes) = on_chain_modules.remove(&(address, module)) else { - return Err(SourceVerificationError::OnChainDependencyNotFound { + errors.push(SourceVerificationError::OnChainDependencyNotFound { package, module, - }) + }); + continue; }; // compare local bytecode to on-chain bytecode to ensure integrity of our // dependencies if local_bytes != on_chain_bytes { - return Err(SourceVerificationError::ModuleBytecodeMismatch { + errors.push(SourceVerificationError::ModuleBytecodeMismatch { address, package, module, @@ -182,7 +212,11 @@ impl<'a> BytecodeSourceVerifier<'a> { } if let Some(((address, module), _)) = on_chain_modules.into_iter().next() { - return Err(SourceVerificationError::LocalDependencyNotFound { address, module }); + errors.push(SourceVerificationError::LocalDependencyNotFound { address, module }); + } + + if !errors.is_empty() { + return Err(AggregateSourceVerificationError(errors)); } Ok(()) diff --git a/crates/sui-source-validation/src/tests.rs b/crates/sui-source-validation/src/tests.rs index 1834af41118cb..1b5c7c969c9f6 100644 --- a/crates/sui-source-validation/src/tests.rs +++ b/crates/sui-source-validation/src/tests.rs @@ -453,6 +453,66 @@ async fn module_bytecode_mismatch() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn multiple_failures() -> anyhow::Result<()> { + let mut cluster = TestClusterBuilder::new().build().await?; + let sender = cluster.get_address_0(); + let context = &mut cluster.wallet; + let mut stable_addrs = HashMap::new(); + + // Publish package `b::b` on-chain without c.move. + let b_ref = { + let fixtures = tempfile::tempdir()?; + let b_src = copy_package(&fixtures, "b", [("b", SuiAddress::ZERO)]).await?; + tokio::fs::remove_file(b_src.join("sources").join("c.move")).await?; + publish_package(context, sender, b_src).await + }; + + // Publish package `c::c` on-chain, unmodified. + let c_ref = { + let fixtures = tempfile::tempdir()?; + let c_src = copy_package(&fixtures, "c", [("c", SuiAddress::ZERO)]).await?; + publish_package(context, sender, c_src).await + }; + + // Compile local package `d` that references: + // - `b::b` (c.move exists locally but not on chain => error) + // - `c::c` (d.move exists on-chain but we delete it locally before compiling => error) + let d_pkg = { + let fixtures = tempfile::tempdir()?; + let b_id = b_ref.0.into(); + let c_id = c_ref.0.into(); + stable_addrs.insert(b_id, ""); + stable_addrs.insert(c_id, ""); + copy_package(&fixtures, "b", [("b", b_id)]).await?; + let c_src = copy_package(&fixtures, "c", [("c", c_id)]).await?; + let d_src = copy_package( + &fixtures, + "d", + [("d", SuiAddress::ZERO), ("b", b_id), ("c", c_id)], + ) + .await?; + tokio::fs::remove_file(c_src.join("sources").join("d.move")).await?; // delete local module in `c` + compile_package(d_src) + }; + + let client = context.get_client().await?; + let verifier = BytecodeSourceVerifier::new(client.read_api(), false); + + let Err(err) = verifier.verify_package_deps(&d_pkg.package).await else { + panic!("Expected verification to fail"); + }; + + let expected = expect![[r#" + Multiple source verification errors found: + + - On-chain version of dependency b::c was not found. + - Local version of dependency ::d was not found."#]]; + expected.assert_eq(&sanitize_id(err.to_string(), &stable_addrs)); + + Ok(()) +} + /// Compile the package at absolute path `package`. fn compile_package(package: impl AsRef) -> CompiledPackage { sui_framework::build_move_package(package.as_ref(), BuildConfig::new_for_testing()).unwrap()