From 4daf3b58e5e9520bf91cecbcb66f1ebaa9280455 Mon Sep 17 00:00:00 2001 From: Wolfgang Grieskamp Date: Wed, 18 Oct 2023 22:19:25 -0700 Subject: [PATCH] Adding combinatorial proptests to test runtime access specifiers. --- crates/aptos-admin-service/src/server/mod.rs | 4 +- .../move/move-binary-format/Cargo.toml | 1 + .../src/file_format_common.rs | 9 +- .../src/acquires_list_verifier.rs | 23 +-- .../src/bytecode_generator.rs | 58 ++++-- .../function_generator.rs | 16 +- .../file_format_generator/module_generator.rs | 70 ++++++- .../tests/bytecode-generator/fields.exp | 36 ++++ .../tests/bytecode-generator/fields.move | 12 ++ .../checking/access_specifiers/access_err.exp | 6 - .../access_control/dynamic.move | 1 - .../access_control/generic.exp | 18 ++ .../access_control/generic.move | 29 +++ .../access_control/negation.exp | 29 +++ .../access_control/negation.move | 34 +++ .../access_control/wildcard.exp | 35 ++++ .../access_control/wildcard.move | 46 +++++ .../move-model/src/builder/exp_builder.rs | 37 +++- third_party/move/move-vm/types/Cargo.toml | 1 + .../runtime_access_specifiers_prop_tests.txt | 9 + .../move/move-vm/types/src/loaded_data/mod.rs | 2 + .../loaded_data/runtime_access_specifier.rs | 3 +- .../runtime_access_specifiers_prop_tests.rs | 194 ++++++++++++++++++ .../transactional-test-runner/Cargo.toml | 2 +- .../move-resource-viewer/src/resolver.rs | 16 +- 25 files changed, 625 insertions(+), 66 deletions(-) create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.exp create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.move create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.exp create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.move create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.exp create mode 100644 third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.move create mode 100644 third_party/move/move-vm/types/proptest-regressions/loaded_data/runtime_access_specifiers_prop_tests.txt create mode 100644 third_party/move/move-vm/types/src/loaded_data/runtime_access_specifiers_prop_tests.rs diff --git a/crates/aptos-admin-service/src/server/mod.rs b/crates/aptos-admin-service/src/server/mod.rs index 41e3f21dd8e9c..8968cad634abc 100644 --- a/crates/aptos-admin-service/src/server/mod.rs +++ b/crates/aptos-admin-service/src/server/mod.rs @@ -4,9 +4,11 @@ use crate::server::utils::reply_with_status; use aptos_config::config::NodeConfig; use aptos_logger::info; +#[cfg(target_os = "linux")] +use hyper::Method; use hyper::{ service::{make_service_fn, service_fn}, - Body, Method, Request, Response, Server, StatusCode, + Body, Request, Response, Server, StatusCode, }; use std::{ convert::Infallible, diff --git a/third_party/move/move-binary-format/Cargo.toml b/third_party/move/move-binary-format/Cargo.toml index 6dca0893990eb..28f5bbf4c02fc 100644 --- a/third_party/move/move-binary-format/Cargo.toml +++ b/third_party/move/move-binary-format/Cargo.toml @@ -31,3 +31,4 @@ serde_json = "1.0.64" [features] default = [] fuzzing = ["proptest", "proptest-derive", "arbitrary", "move-core-types/fuzzing"] +testing = [] diff --git a/third_party/move/move-binary-format/src/file_format_common.rs b/third_party/move/move-binary-format/src/file_format_common.rs index 044a6731804b3..c1ac21f7298ff 100644 --- a/third_party/move/move-binary-format/src/file_format_common.rs +++ b/third_party/move/move-binary-format/src/file_format_common.rs @@ -507,8 +507,13 @@ pub(crate) mod versioned_data { }, }; if version == 0 || version > u32::min(max_version, VERSION_MAX) { - return Err(PartialVMError::new(StatusCode::UNKNOWN_VERSION)); - } else if version == VERSION_NEXT && !cfg!(test) && !cfg!(feature = "fuzzing") { + return Err(PartialVMError::new(StatusCode::UNKNOWN_VERSION) + .with_message(format!("bytecode version {} unsupported", version))); + } else if version == VERSION_NEXT + && !cfg!(test) + && !cfg!(feature = "testing") + && !cfg!(feature = "fuzzing") + { return Err( PartialVMError::new(StatusCode::UNKNOWN_VERSION).with_message(format!( "bytecode version {} only allowed in test code", diff --git a/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs b/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs index 9aa8b6d4419f9..60e76973a7e95 100644 --- a/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs +++ b/third_party/move/move-bytecode-verifier/src/acquires_list_verifier.rs @@ -19,7 +19,6 @@ use move_binary_format::{ Bytecode, CodeOffset, CompiledModule, FunctionDefinition, FunctionDefinitionIndex, FunctionHandle, FunctionHandleIndex, StructDefinitionIndex, }, - file_format_common::VERSION_7, safe_unwrap, }; use move_core_types::vm_status::StatusCode; @@ -183,12 +182,9 @@ impl<'a> AcquiresVerifier<'a> { let function_handle = self.module.function_handle_at(fh_idx); let mut function_acquired_resources = self.function_acquired_resources(function_handle, fh_idx); - if self.module.version < VERSION_7 { - // TODO: consolidate acquires with access control and borrow analysis - for acquired_resource in &function_acquired_resources { - if !self.annotated_acquires.contains(acquired_resource) { - return Err(self.error(StatusCode::MISSING_ACQUIRES_ANNOTATION, offset)); - } + for acquired_resource in &function_acquired_resources { + if !self.annotated_acquires.contains(acquired_resource) { + return Err(self.error(StatusCode::MISSING_ACQUIRES_ANNOTATION, offset)); } } self.actual_acquires @@ -201,16 +197,11 @@ impl<'a> AcquiresVerifier<'a> { sd_idx: StructDefinitionIndex, offset: CodeOffset, ) -> PartialVMResult<()> { - if self.module.version < VERSION_7 { - // TODO: consolidate acquires with access control and borrow analysis - if self.annotated_acquires.contains(&sd_idx) { - self.actual_acquires.insert(sd_idx); - Ok(()) - } else { - Err(self.error(StatusCode::MISSING_ACQUIRES_ANNOTATION, offset)) - } - } else { + if self.annotated_acquires.contains(&sd_idx) { + self.actual_acquires.insert(sd_idx); Ok(()) + } else { + Err(self.error(StatusCode::MISSING_ACQUIRES_ANNOTATION, offset)) } } diff --git a/third_party/move/move-compiler-v2/src/bytecode_generator.rs b/third_party/move/move-compiler-v2/src/bytecode_generator.rs index e020f1171297d..a19b3b348702c 100644 --- a/third_party/move/move-compiler-v2/src/bytecode_generator.rs +++ b/third_party/move/move-compiler-v2/src/bytecode_generator.rs @@ -521,10 +521,26 @@ impl<'env> Generator<'env> { self.gen_op_call(targets, id, BytecodeOperation::Pack(*mid, *sid, inst), args) }, Operation::Select(mid, sid, fid) => { - let inst = self.env().get_node_instantiation(id); let target = self.require_unary_target(id, targets); let arg = self.require_unary_arg(id, args); - self.gen_select(target, id, mid.qualified_inst(*sid, inst), *fid, &arg) + // Get the instantiation of the struct. It's not contained in the select + // expression but in the type of it's operand. + if let Some((_, inst)) = self + .env() + .get_node_type(arg.node_id()) + .skip_reference() + .get_struct(self.env()) + { + self.gen_select( + target, + id, + mid.qualified_inst(*sid, inst.to_vec()), + *fid, + &arg, + ) + } else { + self.internal_error(id, "inconsistent type in select expression") + } }, Operation::Exists(None) => { let inst = self.env().get_node_instantiation(id); @@ -788,13 +804,12 @@ impl<'env> Generator<'env> { match exp.as_ref() { ExpData::Temporary(_, temp) => *temp, ExpData::LocalVar(id, sym) => self.find_local(*id, *sym), - ExpData::Call(_, Operation::Select(..), _) if self.reference_mode() => { + ExpData::Call(id, Operation::Select(..), _) if self.reference_mode() => { // In reference mode, a selection is interpreted as selecting a reference to the // field. - let id = exp.node_id(); let ty = Type::Reference( self.reference_mode_kind, - Box::new(self.env().get_node_type(id)), + Box::new(self.env().get_node_type(*id)), ); let temp = self.new_temp(ty); self.gen(vec![temp], exp); @@ -844,10 +859,10 @@ impl<'env> Generator<'env> { impl<'env> Generator<'env> { fn gen_borrow(&mut self, target: TempIndex, id: NodeId, kind: ReferenceKind, arg: &Exp) { match arg.as_ref() { - ExpData::Call(_arg_id, Operation::Select(mid, sid, fid), args) => { + ExpData::Call(arg_id, Operation::Select(mid, sid, fid), args) => { return self.gen_borrow_field( target, - id, + *arg_id, kind, mid.qualified(*sid), *fid, @@ -924,13 +939,28 @@ impl<'env> Generator<'env> { // Currently no conditions for immutable, so we do allow `&fun().field`. }, } - let inst = self.env().get_node_instantiation(id); - self.emit_call( - id, - vec![target], - BytecodeOperation::BorrowField(struct_id.module_id, struct_id.id, inst, field_offset), - vec![temp], - ); + // Get instantiation of field. It is not contained in the select expression but in the + // type of its operand. + if let Some((_, inst)) = self + .env() + .get_node_type(oper.node_id()) + .skip_reference() + .get_struct(self.env()) + { + self.emit_call( + id, + vec![target], + BytecodeOperation::BorrowField( + struct_id.module_id, + struct_id.id, + inst.to_vec(), + field_offset, + ), + vec![temp], + ); + } else { + self.internal_error(id, "inconsistent type in select expression") + } } } diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs index 2f76bab4e21a0..bb396d335fa3c 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs @@ -84,7 +84,12 @@ struct LabelInfo { impl<'a> FunctionGenerator<'a> { /// Runs the function generator for the given function. - pub fn run<'b>(gen: &'a mut ModuleGenerator, ctx: &'b ModuleContext, fun_env: FunctionEnv<'b>) { + pub fn run<'b>( + gen: &'a mut ModuleGenerator, + ctx: &'b ModuleContext, + fun_env: FunctionEnv<'b>, + acquires_list: &BTreeSet, + ) { let loc = fun_env.get_loc(); let function = gen.function_index(ctx, &loc, &fun_env); let visibility = fun_env.visibility(); @@ -111,11 +116,18 @@ impl<'a> FunctionGenerator<'a> { } else { (gen, None) }; + let acquires_global_resources = acquires_list + .iter() + .map(|id| { + let struct_env = fun_env.module_env.get_struct(*id); + gen.struct_def_index(ctx, &struct_env.get_loc(), &struct_env) + }) + .collect(); let def = FF::FunctionDefinition { function, visibility, is_entry: fun_env.is_entry(), - acquires_global_resources: vec![], + acquires_global_resources, code, }; ctx.checked_bound( diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs index 754bf4962e852..3d02e89049157 100644 --- a/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs +++ b/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs @@ -27,10 +27,11 @@ use move_model::{ ty::{PrimitiveType, ReferenceKind, Type}, }; use move_stackless_bytecode::{ - function_target_pipeline::FunctionTargetsHolder, stackless_bytecode::Constant, + function_target_pipeline::{FunctionTargetsHolder, FunctionVariant}, + stackless_bytecode::{Bytecode, Constant, Operation}, }; use move_symbol_pool::symbol as IR_SYMBOL; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; /// Internal state of the module code generator #[derive(Debug)] @@ -144,8 +145,11 @@ impl ModuleGenerator { for struct_env in module_env.get_structs() { self.gen_struct(ctx, &struct_env) } + + let acquires_map = ctx.generate_acquires_map(module_env); for fun_env in module_env.get_functions() { - FunctionGenerator::run(self, ctx, fun_env); + let acquires_list = &acquires_map[&fun_env.get_id()]; + FunctionGenerator::run(self, ctx, fun_env, acquires_list); } } @@ -766,3 +770,63 @@ impl<'env> ModuleContext<'env> { s.display(self.env.symbol_pool()).to_string() } } + +impl<'env> ModuleContext<'env> { + /// Acquires analysis. This is temporary until we have the full reference analysis. + fn generate_acquires_map(&self, module: &ModuleEnv) -> BTreeMap> { + // Compute map with direct usage of resources + let mut usage_map = module + .get_functions() + .into_iter() + .map(|f| (f.get_id(), self.get_function_acquires(&f))) + .collect::>(); + // Now run a fixed-point loop: add resources used by called functions until there are no + // changes. + loop { + let mut changes = false; + for fun in module.get_functions() { + if let Some(callees) = fun.get_called_functions() { + let mut usage = usage_map[&fun.get_id()].clone(); + let mut local_changes = false; + for callee in callees { + if callee.module_id == module.get_id() { + let count = usage.len(); + usage.extend(usage_map[&callee.id].iter().cloned()); + if usage.len() > count { + local_changes = true; + } + } + } + if local_changes { + *usage_map.get_mut(&fun.get_id()).unwrap() = usage; + changes = true; + } + } + } + if !changes { + break; + } + } + usage_map + } + + fn get_function_acquires(&self, fun: &FunctionEnv) -> BTreeSet { + let mut result = BTreeSet::new(); + let target = self.targets.get_target(fun, &FunctionVariant::Baseline); + for bc in target.get_bytecode() { + use Bytecode::*; + use Operation::*; + match bc { + Call(_, _, MoveFrom(mid, sid, ..), ..) + | Call(_, _, BorrowGlobal(mid, sid, ..), ..) + | Call(_, _, GetGlobal(mid, sid, ..), ..) + if *mid == fun.module_env.get_id() => + { + result.insert(*sid); + }, + _ => {}, + } + } + result + } +} diff --git a/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp b/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp index 4ed3af5f47963..70d793c4bdbb5 100644 --- a/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp +++ b/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp @@ -3,16 +3,25 @@ module 0x42::fields { struct T { h: u64, } + struct G { + f: #0, + } struct S { f: u64, g: fields::T, } + private fun read_generic_val(x: fields::G): u64 { + select fields::G.f(x) + } private fun read_ref(x: &fields::S): u64 { select fields::T.h(select fields::S.g(x)) } private fun read_val(x: fields::S): u64 { select fields::T.h(select fields::S.g(x)) } + private fun write_generic_val(x: &mut fields::G,v: u64) { + select fields::G.f(x) = v + } private fun write_local_direct(): fields::S { { let x: fields::S = pack fields::S(0, pack fields::T(0)); @@ -38,12 +47,18 @@ module 0x42::fields { select fields::T.h(select fields::S.g(x)) = 42; x } + spec fun $read_generic_val(x: fields::G): u64 { + select fields::G.f(x) + } spec fun $read_ref(x: fields::S): u64 { select fields::T.h(select fields::S.g(x)) } spec fun $read_val(x: fields::S): u64 { select fields::T.h(select fields::S.g(x)) } + spec fun $write_generic_val(x: fields::G,v: u64) { + select fields::G.f(x) = v + } spec fun $write_local_direct(): fields::S; spec fun $write_local_via_ref(): fields::S; spec fun $write_param(x: &mut fields::S); @@ -52,6 +67,18 @@ module 0x42::fields { ============ initial bytecode ================ +[variant baseline] +fun fields::read_generic_val($t0: fields::G): u64 { + var $t1: u64 + var $t2: &fields::G + var $t3: &u64 + 0: $t2 := borrow_local($t0) + 1: $t3 := borrow_field>.f($t2) + 2: $t1 := read_ref($t3) + 3: return $t1 +} + + [variant baseline] fun fields::read_ref($t0: &fields::S): u64 { var $t1: u64 @@ -78,6 +105,15 @@ fun fields::read_val($t0: fields::S): u64 { } +[variant baseline] +fun fields::write_generic_val($t0: &mut fields::G, $t1: u64) { + var $t2: &mut u64 + 0: $t2 := borrow_field>.f($t0) + 1: write_ref($t2, $t1) + 2: return () +} + + [variant baseline] fun fields::write_local_direct(): fields::S { var $t0: fields::S diff --git a/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.move b/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.move index da6bea9d1d449..3d6d69aa4f304 100644 --- a/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.move +++ b/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.move @@ -9,6 +9,10 @@ module 0x42::fields { h: u64 } + struct G { + f: X + } + fun read_val(x: S): u64 { x.g.h } @@ -38,4 +42,12 @@ module 0x42::fields { x.g.h = 42; x } + + fun read_generic_val(x: G): u64 { + x.f + } + + fun write_generic_val(x: &mut G, v: u64) { + x.f = v + } } diff --git a/third_party/move/move-compiler-v2/tests/checking/access_specifiers/access_err.exp b/third_party/move/move-compiler-v2/tests/checking/access_specifiers/access_err.exp index d509e3ec57029..2d07719a5be35 100644 --- a/third_party/move/move-compiler-v2/tests/checking/access_specifiers/access_err.exp +++ b/third_party/move/move-compiler-v2/tests/checking/access_specifiers/access_err.exp @@ -18,12 +18,6 @@ error: invalid access specifier: a wildcard cannot be followed by a non-wildcard 12 │ fun f3() acquires 0x42::*::S { │ ^^^^^^^^^^^ -error: type argument count mismatch (expected 1 but got 0) - ┌─ tests/checking/access_specifiers/access_err.move:15:23 - │ -15 │ fun f4() acquires G { - │ ^^ - error: undeclared `m::y` ┌─ tests/checking/access_specifiers/access_err.move:18:35 │ diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/dynamic.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/dynamic.move index c5638f2703190..d6d80cf80a1fd 100644 --- a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/dynamic.move +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/dynamic.move @@ -34,4 +34,3 @@ module 0x42::test { //# run --verbose --args @0x1 -- 0x42::test::fail1 //# run --verbose --signers 0x1 -- 0x42::test::fail2 - diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.exp new file mode 100644 index 0000000000000..a0f1c4c20301a --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.exp @@ -0,0 +1,18 @@ +processed 5 tasks + +task 2 'run'. lines 25-25: +return values: true + +task 3 'run'. lines 27-27: +return values: true + +task 4 'run'. lines 29-29: +Error: Function execution failed with VMError: { + message: not allowed to perform `reads 0x42::test::R(@0x1)`, + major_status: ACCESS_DENIED, + sub_status: None, + location: 0x42::test, + indices: [], + offsets: [(FunctionDefinitionIndex(0), 1)], + exec_state: Some(ExecutionState { stack_trace: [] }), +} diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.move new file mode 100644 index 0000000000000..d786c36236502 --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/generic.move @@ -0,0 +1,29 @@ +//# publish +module 0x42::test { + + struct R has key, drop { value: T } + + fun init(s: &signer) { + move_to(s, R{value: true}); + } + + fun ok1(): bool reads R { + borrow_global>(@0x1).value + } + + fun ok2(): bool reads R { + borrow_global>(@0x1).value + } + + fun fail1(): bool reads R { + borrow_global>(@0x1).value + } +} + +//# run --verbose --signers 0x1 -- 0x42::test::init + +//# run --verbose -- 0x42::test::ok1 + +//# run --verbose -- 0x42::test::ok2 + +//# run --verbose -- 0x42::test::fail1 diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.exp new file mode 100644 index 0000000000000..3ea63bb986779 --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.exp @@ -0,0 +1,29 @@ +processed 6 tasks + +task 2 'run'. lines 28-28: +return values: true + +task 3 'run'. lines 30-30: +return values: true + +task 4 'run'. lines 32-32: +Error: Function execution failed with VMError: { + message: not allowed to perform `reads 0x42::test::R(@0x1)`, + major_status: ACCESS_DENIED, + sub_status: None, + location: 0x42::test, + indices: [], + offsets: [(FunctionDefinitionIndex(0), 1)], + exec_state: Some(ExecutionState { stack_trace: [] }), +} + +task 5 'run'. lines 34-34: +Error: Function execution failed with VMError: { + message: not allowed to perform `reads 0x42::test::R(@0x1)`, + major_status: ACCESS_DENIED, + sub_status: None, + location: 0x42::test, + indices: [], + offsets: [(FunctionDefinitionIndex(1), 1)], + exec_state: Some(ExecutionState { stack_trace: [] }), +} diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.move new file mode 100644 index 0000000000000..a6057f4b6f062 --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/negation.move @@ -0,0 +1,34 @@ +//# publish +module 0x42::test { + struct R has key, drop { value: bool } + + fun init(s: &signer) { + move_to(s, R{value: true}); + } + + fun ok1(): bool reads 0x42::*::*, !reads 0x43::*::* { + borrow_global(@0x1).value + } + + fun ok2(): bool acquires *, !reads 0x43::*::* { + borrow_global(@0x1).value + } + + fun fail1(): bool !reads 0x42::*::* { + borrow_global(@0x1).value + } + + fun fail2(): bool !reads *(0x1) { + borrow_global(@0x1).value + } +} + +//# run --verbose --signers 0x1 -- 0x42::test::init + +//# run --verbose -- 0x42::test::ok1 + +//# run --verbose -- 0x42::test::ok2 + +//# run --verbose -- 0x42::test::fail1 + +//# run --verbose -- 0x42::test::fail2 diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.exp new file mode 100644 index 0000000000000..67dda0b118903 --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.exp @@ -0,0 +1,35 @@ +processed 8 tasks + +task 2 'run'. lines 36-36: +return values: true + +task 3 'run'. lines 38-38: +return values: true + +task 4 'run'. lines 40-40: +return values: true + +task 5 'run'. lines 42-42: +return values: true + +task 6 'run'. lines 44-44: +Error: Function execution failed with VMError: { + message: not allowed to perform `reads 0x42::test::R(@0x1)`, + major_status: ACCESS_DENIED, + sub_status: None, + location: 0x42::test, + indices: [], + offsets: [(FunctionDefinitionIndex(0), 1)], + exec_state: Some(ExecutionState { stack_trace: [] }), +} + +task 7 'run'. lines 46-46: +Error: Function execution failed with VMError: { + message: not allowed to perform `reads 0x42::test::R(@0x1)`, + major_status: ACCESS_DENIED, + sub_status: None, + location: 0x42::test, + indices: [], + offsets: [(FunctionDefinitionIndex(1), 1)], + exec_state: Some(ExecutionState { stack_trace: [] }), +} diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.move new file mode 100644 index 0000000000000..7078634a6c432 --- /dev/null +++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/access_control/wildcard.move @@ -0,0 +1,46 @@ +//# publish +module 0x42::test { + struct R has key, drop { value: bool } + + fun init(s: &signer) { + move_to(s, R{value: true}); + } + + fun ok1(): bool reads 0x42::*::* { + borrow_global(@0x1).value + } + + fun ok2(): bool reads 0x42::test::* { + borrow_global(@0x1).value + } + + fun ok3(): bool reads 0x42::test::*(*) { + borrow_global(@0x1).value + } + + fun ok4(): bool reads *(0x1) { + borrow_global(@0x1).value + } + + fun fail1(): bool reads 0x43::*::* { + borrow_global(@0x1).value + } + + fun fail2(): bool reads *(0x2) { + borrow_global(@0x1).value + } +} + +//# run --verbose --signers 0x1 -- 0x42::test::init + +//# run --verbose -- 0x42::test::ok1 + +//# run --verbose -- 0x42::test::ok2 + +//# run --verbose -- 0x42::test::ok3 + +//# run --verbose -- 0x42::test::ok4 + +//# run --verbose -- 0x42::test::fail1 + +//# run --verbose -- 0x42::test::fail2 diff --git a/third_party/move/move-model/src/builder/exp_builder.rs b/third_party/move/move-model/src/builder/exp_builder.rs index d20bc9502885b..4b2b76dcaea7b 100644 --- a/third_party/move/move-model/src/builder/exp_builder.rs +++ b/third_party/move/move-model/src/builder/exp_builder.rs @@ -903,8 +903,6 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo if is_wildcard(resource) { ResourceSpecifier::DeclaredInModule(module_id) } else { - // Construct an expansion type so we can feed it through the standard translation - // process. let mident = sp(specifier.loc, EA::ModuleIdent_ { address: *address, module: *module, @@ -913,16 +911,33 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo specifier.loc, EA::ModuleAccess_::ModuleAccess(mident, *resource), ); - let ety = sp( - specifier.loc, - EA::Type_::Apply(maccess, type_args.as_ref().cloned().unwrap_or_default()), - ); - let ty = self.translate_type(&ety); - if let Type::Struct(mid, sid, inst) = ty { - ResourceSpecifier::Resource(mid.qualified_inst(sid, inst)) + let sym = self.parent.module_access_to_qualified(&maccess); + if let Type::Struct(mid, sid, _) = self.parent.parent.lookup_type(&loc, &sym) { + if type_args.is_none() { + // If no type args are provided, we assume this is a partial type without + // instantiation + ResourceSpecifier::Resource(mid.qualified_inst(sid, vec![])) + } else { + // Otherwise construct an expansion type so we can feed it through the standard translation + // process. + let ety = sp( + specifier.loc, + EA::Type_::Apply( + maccess, + type_args.as_ref().cloned().unwrap_or_default(), + ), + ); + let ty = self.translate_type(&ety); + if let Type::Struct(mid, sid, inst) = ty { + ResourceSpecifier::Resource(mid.qualified_inst(sid, inst)) + } else { + // errors reported + debug_assert!(self.parent.parent.env.has_errors()); + ResourceSpecifier::Any + } + } } else { - // errors reported - debug_assert!(self.parent.parent.env.has_errors()); + // error reported ResourceSpecifier::Any } } diff --git a/third_party/move/move-vm/types/Cargo.toml b/third_party/move/move-vm/types/Cargo.toml index 870e9097fc4a7..8aeffdd0d65e0 100644 --- a/third_party/move/move-vm/types/Cargo.toml +++ b/third_party/move/move-vm/types/Cargo.toml @@ -23,6 +23,7 @@ move-binary-format = { path = "../../move-binary-format" } move-core-types = { path = "../../move-core/types" } [dev-dependencies] +move-binary-format = { path = "../../move-binary-format", features = ["fuzzing"] } proptest = "1.0.0" [features] diff --git a/third_party/move/move-vm/types/proptest-regressions/loaded_data/runtime_access_specifiers_prop_tests.txt b/third_party/move/move-vm/types/proptest-regressions/loaded_data/runtime_access_specifiers_prop_tests.txt new file mode 100644 index 0000000000000..c4284eca12b58 --- /dev/null +++ b/third_party/move/move-vm/types/proptest-regressions/loaded_data/runtime_access_specifiers_prop_tests.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 909d52933dc057f54814b28bd6ed88504cee077ece2e6d70252adeeab2ba9f58 # shrinks to access = AccessInstance { kind: Reads, resource: StructIdentifier { module: ModuleId { address: 0000000000000000000000000000000000000000000000000000000000000001, name: Identifier("ac") }, name: Identifier("ac") }, instance: [], address: 0000000000000000000000000000000000000000000000000000000000000001 }, s1 = Any, s2 = Any +cc dba42d5ed4d6a925756fb4ea8f829bed5a209f2be288885b453b352eec92ce5f # shrinks to access = AccessInstance { kind: Reads, resource: StructIdentifier { module: ModuleId { address: 0000000000000000000000000000000000000000000000000000000000000001, name: Identifier("ac") }, name: Identifier("ac") }, instance: [], address: 0000000000000000000000000000000000000000000000000000000000000001 }, s1 = Any, s2 = Any +cc b95bd44e13064041a78409ce4f11d729f189756de014c143b909e7eec2b8c81f # shrinks to access = AccessInstance { kind: Reads, resource: StructIdentifier { module: ModuleId { address: 0000000000000000000000000000000000000000000000000000000000000002, name: Identifier("ac") }, name: Identifier("ac") }, instance: [], address: 0000000000000000000000000000000000000000000000000000000000000002 }, s1 = Constraint([AccessSpecifierClause { kind: Acquires, resource: Any, address: Literal(0000000000000000000000000000000000000000000000000000000000000001) }], []), s2 = Constraint([AccessSpecifierClause { kind: Acquires, resource: Any, address: Any }], []) diff --git a/third_party/move/move-vm/types/src/loaded_data/mod.rs b/third_party/move/move-vm/types/src/loaded_data/mod.rs index 9ffb2e6dea889..50ceaba95ca0e 100644 --- a/third_party/move/move-vm/types/src/loaded_data/mod.rs +++ b/third_party/move/move-vm/types/src/loaded_data/mod.rs @@ -6,4 +6,6 @@ //! This module contains the loaded definition of code data used in runtime. pub mod runtime_access_specifier; +#[cfg(test)] +mod runtime_access_specifiers_prop_tests; pub mod runtime_types; diff --git a/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs b/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs index aaab80a981cee..7d014114a420d 100644 --- a/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs +++ b/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifier.rs @@ -1,4 +1,3 @@ -// Copyright (c) The Diem Core Contributors // Copyright (c) The Move Contributors // SPDX-License-Identifier: Apache-2.0 @@ -436,7 +435,7 @@ impl AddressSpecifier { use AddressSpecifier::*; match (self, other) { (Any, _) => true, - (_, Any) => true, + (_, Any) => false, (Literal(a1), Literal(a2)) => a1 == a2, (_, _) => { // Eval should be specialized away when subsumes is called. diff --git a/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifiers_prop_tests.rs b/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifiers_prop_tests.rs new file mode 100644 index 0000000000000..59286badda508 --- /dev/null +++ b/third_party/move/move-vm/types/src/loaded_data/runtime_access_specifiers_prop_tests.rs @@ -0,0 +1,194 @@ +// Copyright (c) The Move Contributors +// SPDX-License-Identifier: Apache-2.0 + +use crate::loaded_data::{ + runtime_access_specifier::{ + AccessInstance, AccessSpecifier, AccessSpecifierClause, AddressSpecifier, ResourceSpecifier, + }, + runtime_types::{StructIdentifier, Type}, +}; +use move_binary_format::file_format::AccessKind; +use move_core_types::{ + account_address::AccountAddress, identifier::Identifier, language_storage::ModuleId, +}; +use proptest::{collection::vec, prelude::*}; + +proptest! { + #![proptest_config(ProptestConfig{cases: 5000, verbose: 1, ..ProptestConfig::default()})] + + /// Test in all combinations the semi-lattice properties of the join and subsumes. + /// Together with testing basic membership, this gives extensive coverage of the operators. + #[test] + fn access_specifier_semi_lattice_properties( + access in access_instance_strategy(), + s1 in access_specifier_strategy(4, 3), + s2 in access_specifier_strategy(4, 3) + ) { + //eprint!("."); + if s1.enables(&access) && s2.enables(&access) { + assert!(s1.join(&s2).enables(&access)) + } else { + assert!(!s1.join(&s2).enables(&access)) + } + if s1.subsumes(&s2).unwrap_or_default() && s2.enables(&access) { + assert!(s1.enables(&access)) + } + } + + /// Test membership, by constructing all combinations of specifiers derivable from a given + /// instance. + #[test] + fn access_specifier_enables( + (access1, clause1) in access_to_matching_specifier_clause(access_instance_strategy()), + (access2, clause2) in access_to_matching_specifier_clause(access_instance_strategy()), + ) { + //eprint!("."); + let clauses = vec![clause1, clause2]; + let incl = AccessSpecifier::Constraint(clauses.clone(), vec![]); + let incl_excl = AccessSpecifier::Constraint(clauses.clone(), clauses.clone()); + let excl = AccessSpecifier::Constraint(vec![], clauses.clone()); + assert!(incl.enables(&access1)); + assert!(incl.enables(&access2)); + assert!(incl.join(&incl).enables(&access1)); + assert!(incl.join(&incl).enables(&access2)); + assert!(!incl_excl.enables(&access1)); + assert!(!incl_excl.enables(&access2)); + assert!(!excl.enables(&access1)); + assert!(!excl.enables(&access2)); + } +} + +fn access_instance_strategy() -> impl Strategy { + ( + any::(), + struct_id_strategy(), + type_args_strategy(), + address_strategy(), + ) + .prop_map(|(kind, resource, instance, address)| AccessInstance { + kind, + resource, + instance, + address, + }) +} +fn access_specifier_strategy( + incl_size: usize, + excl_size: usize, +) -> impl Strategy { + prop_oneof![ + Just(AccessSpecifier::Any), + ( + vec(access_specifier_clause_strategy(), 0..incl_size), + vec(access_specifier_clause_strategy(), 0..excl_size), + ) + .prop_map(|(incls, excls)| AccessSpecifier::Constraint(incls, excls)) + ] +} + +fn access_specifier_clause_strategy() -> impl Strategy { + ( + any::(), + resource_specifier_strategy(), + address_specifier_strategy(), + ) + .prop_map(|(kind, resource, address)| AccessSpecifierClause { + kind, + resource, + address, + }) +} + +fn resource_specifier_strategy() -> impl Strategy { + prop_oneof![ + Just(ResourceSpecifier::Any), + address_strategy().prop_map(ResourceSpecifier::DeclaredAtAddress), + module_id_strategy().prop_map(ResourceSpecifier::DeclaredInModule), + struct_id_strategy().prop_map(ResourceSpecifier::Resource), + (struct_id_strategy(), type_args_strategy()) + .prop_map(|(s, ts)| ResourceSpecifier::ResourceInstantiation(s, ts)), + ] +} + +fn address_specifier_strategy() -> impl Strategy { + prop_oneof![ + Just(AddressSpecifier::Any), + address_strategy().prop_map(AddressSpecifier::Literal) // Skip Eval as it is not appearing subsumes and join + ] +} + +fn type_args_strategy() -> impl Strategy> { + prop_oneof![ + Just(vec![]), + Just(vec![Type::U8]), + Just(vec![Type::U16, Type::U32]) + ] +} + +fn struct_id_strategy() -> impl Strategy { + (module_id_strategy(), identifier_strategy()) + .prop_map(|(module, name)| StructIdentifier { module, name }) +} + +fn module_id_strategy() -> impl Strategy { + (address_strategy(), identifier_strategy()).prop_map(|(a, i)| ModuleId::new(a, i)) +} + +fn identifier_strategy() -> impl Strategy { + "[a-b]{1}[c-d]{1}".prop_map(|s| Identifier::new(s).unwrap()) +} + +fn address_strategy() -> impl Strategy { + prop_oneof![ + Just(AccountAddress::from_str_strict("0x1").unwrap()), + Just(AccountAddress::from_str_strict("0x2").unwrap()), + Just(AccountAddress::from_str_strict("0x3").unwrap()) + ] +} + +/// Map a strategy of instances to matching access specifier clauses. +fn access_to_matching_specifier_clause( + instances: impl Strategy, +) -> impl Strategy { + instances.prop_flat_map(|inst| { + ( + Just(inst.kind), + resource_to_matching_specifier(Just((inst.resource.clone(), inst.instance.clone()))), + address_to_matching_specifier(Just(inst.address)), + ) + .prop_map(move |(kind, resource, address)| { + (inst.clone(), AccessSpecifierClause { + kind, + resource, + address, + }) + }) + }) +} + +/// Map a strategy of resources to a strategy of specifiers which match them. +fn resource_to_matching_specifier( + resources: impl Strategy)>, +) -> impl Strategy { + resources.prop_flat_map(|(s, ts)| { + prop_oneof![ + Just(ResourceSpecifier::Any), + Just(ResourceSpecifier::DeclaredAtAddress(s.module.address)), + Just(ResourceSpecifier::DeclaredInModule(s.module.clone())), + Just(ResourceSpecifier::Resource(s.clone())), + Just(ResourceSpecifier::ResourceInstantiation(s, ts)) + ] + }) +} + +/// Map a strategy of addresses to a strategy of specifiers which match them. +fn address_to_matching_specifier( + addresses: impl Strategy, +) -> impl Strategy { + addresses.prop_flat_map(|a| { + prop_oneof![ + Just(AddressSpecifier::Any), + Just(AddressSpecifier::Literal(a)) + ] + }) +} diff --git a/third_party/move/testing-infra/transactional-test-runner/Cargo.toml b/third_party/move/testing-infra/transactional-test-runner/Cargo.toml index 86dc3fc6377d8..0ae44d23bd81e 100644 --- a/third_party/move/testing-infra/transactional-test-runner/Cargo.toml +++ b/third_party/move/testing-infra/transactional-test-runner/Cargo.toml @@ -13,7 +13,7 @@ edition = "2021" anyhow = "1.0.52" clap = { version = "4.3.9", features = ["derive"] } colored = "2.0.0" -move-binary-format = { path = "../../move-binary-format" } +move-binary-format = { path = "../../move-binary-format", features = ["testing"] } move-bytecode-source-map = { path = "../../move-ir-compiler/move-bytecode-source-map" } move-bytecode-utils = { path = "../../tools/move-bytecode-utils" } move-bytecode-verifier = { path = "../../move-bytecode-verifier" } diff --git a/third_party/move/tools/move-resource-viewer/src/resolver.rs b/third_party/move/tools/move-resource-viewer/src/resolver.rs index 857d2bcdbfe07..015faf80d9eb4 100644 --- a/third_party/move/tools/move-resource-viewer/src/resolver.rs +++ b/third_party/move/tools/move-resource-viewer/src/resolver.rs @@ -13,6 +13,7 @@ use move_binary_format::{ file_format::{ SignatureToken, StructDefinitionIndex, StructFieldInformation, StructHandleIndex, }, + file_format_common::VERSION_MAX, views::FunctionHandleView, CompiledModule, }; @@ -43,13 +44,14 @@ impl<'a, T: MoveResolver + ?Sized> GetModule for Resolver<'a, T> { .get_module(module_id) .map_err(|e| anyhow!("Error retrieving module {:?}: {:?}", module_id, e))? .ok_or_else(|| anyhow!("Module {:?} can't be found", module_id))?; - let compiled_module = CompiledModule::deserialize(&blob).map_err(|status| { - anyhow!( - "Module {:?} deserialize with error code {:?}", - module_id, - status - ) - })?; + let compiled_module = CompiledModule::deserialize_with_max_version(&blob, VERSION_MAX) + .map_err(|status| { + anyhow!( + "Module {:?} deserialize with error code {:?}", + module_id, + status + ) + })?; Ok(Some(self.cache.insert(module_id.clone(), compiled_module))) } }