Skip to content

Commit

Permalink
Refactor the data_struct validation in the verifier (#1663)
Browse files Browse the repository at this point in the history 
* use the data_struct that owned by the module itself

* remove println macros

* use the internal verified_modules
  • Loading branch information
@steelgeek091
steelgeek091 authored May 10, 2024
1 parent 645cb7b commit 9d1f7b3
Show file tree
Hide file tree
Showing 8 changed files with 258 additions and 50 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

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

2 changes: 1 addition & 1 deletion crates/rooch/src/commands/move_cli/commands/publish.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ impl CommandAction<ExecuteTransactionResponseView> for Publish {
let sorted_modules = sort_by_dependency_order(modules.iter_modules())?;
let resolver = context.get_client().await?;
// Serialize and collect module binaries into bundles
verifier::verify_modules(&sorted_modules, &resolver)?;
for module in sorted_modules {
let module_address = module.self_id().address().to_owned();
if module_address != pkg_address {
Expand All @@ -101,7 +102,6 @@ impl CommandAction<ExecuteTransactionResponseView> for Publish {
pkg_address.clone(),
)));
};
verifier::verify_module(&module, &resolver)?;
let mut binary: Vec<u8> = vec![];
module.serialize(&mut binary)?;
bundles.push(binary);
Expand Down
13 changes: 12 additions & 1 deletion frameworks/moveos-stdlib/src/natives/moveos_stdlib/move_module.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,24 @@ fn native_sort_and_verify_modules_inner(
let module_context = context.extensions_mut().get_mut::<NativeModuleContext>();
let mut module_ids = vec![];
let mut init_identifier = vec![];

let verify_result =
moveos_verifier::verifier::verify_modules(&compiled_modules, module_context.resolver);
match verify_result {
Ok(_) => {}
Err(e) => {
log::info!("modules verification error: {:?}", e);
return Ok(NativeResult::err(cost, E_MODULE_VERIFICATION_ERROR));
}
}

for module in &compiled_modules {
let module_address = *module.self_id().address();

if module_address != account_address {
return Ok(NativeResult::err(cost, E_ADDRESS_NOT_MATCH_WITH_SIGNER));
}
let result = moveos_verifier::verifier::verify_module(module, module_context.resolver);
let result = moveos_verifier::verifier::verify_init_function(module);
match result {
Ok(res) => {
if res {
Expand Down
3 changes: 2 additions & 1 deletion moveos/moveos-verifier/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ move-vm-runtime = { workspace = true }
move-vm-types = { workspace = true }
move-symbol-pool = { workspace = true }

moveos-types = { workspace = true }
moveos-types = { workspace = true }
log = "0.4.21"
16 changes: 16 additions & 0 deletions moveos/moveos-verifier/src/build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,22 @@ pub fn inject_runtime_metadata<P: AsRef<Path>>(
CompiledUnit::Module(named_module) => {
if let Some(module_metadata) = metadata.get(&named_module.module.self_id()) {
if !module_metadata.is_empty() {
log::debug!(
"\n\nstart dump data structs map {:?}",
named_module.module.self_id().to_string()
);
for (k, v) in module_metadata.data_struct_map.iter() {
log::debug!("{:?} -> {:?}", k, v);
}
log::debug!("\n");
for (k, v) in module_metadata.data_struct_func_map.iter() {
log::debug!("{:?} -> {:?}", k, v);
}
log::debug!(
"start dump data structs map {:?}\n\n",
named_module.module.self_id().to_string()
);

let serialized_metadata =
bcs::to_bytes(&module_metadata).expect("BCS for RuntimeModuleMetadata");
named_module.module.metadata.push(Metadata {
Expand Down
91 changes: 69 additions & 22 deletions moveos/moveos-verifier/src/metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -929,11 +929,16 @@ impl<'a> ExtendedChecker<'a> {

impl<'a> ExtendedChecker<'a> {
fn check_data_struct(&mut self, module_env: &ModuleEnv) {
let mut available_data_structs = BTreeMap::new();

for struct_def in module_env.get_structs() {
if is_data_struct_annotation(&struct_def, module_env) {
if is_copy_drop_struct(&struct_def) {
let (error_message, is_allowed) =
check_data_struct_fields(&struct_def, module_env);
let (error_message, is_allowed) = check_data_struct_fields(
&struct_def,
module_env,
&mut available_data_structs,
);
if !is_allowed {
self.env
.error(&struct_def.get_loc(), error_message.as_str());
Expand All @@ -952,6 +957,20 @@ impl<'a> ExtendedChecker<'a> {
}
}

if !available_data_structs.is_empty() {
log::debug!(
"\n\ncheck_data_struct() module {:?} data structs start...",
module_env.get_full_name_str()
);
for (k, v) in available_data_structs.iter() {
log::debug!("{:?} -> {:?}", k, v);
}
log::debug!(
"\n\ncheck_data_struct() module {:?} data structs end...",
module_env.get_full_name_str()
);
}

let verified_module = match module_env.get_verified_module() {
None => {
self.env
Expand All @@ -963,29 +982,30 @@ impl<'a> ExtendedChecker<'a> {

let module_metadata = self.output.entry(verified_module.self_id()).or_default();

let data_struct_map = unsafe {
let result: BTreeMap<String, bool> = GLOBAL_DATA_STRUCT
.iter()
.map(|(key, value)| (key.clone(), *value))
.collect();
result
};
let data_struct_map: BTreeMap<String, bool> = available_data_structs
.iter()
.map(|(key, value)| (key.clone(), *value))
.collect();

module_metadata.data_struct_map = data_struct_map;

check_data_struct_func(self, module_env);
}
}

fn check_data_struct_fields(struct_def: &StructEnv, module_env: &ModuleEnv) -> (String, bool) {
fn check_data_struct_fields(
struct_def: &StructEnv,
module_env: &ModuleEnv,
valid_structs: &mut BTreeMap<String, bool>,
) -> (String, bool) {
let struct_fields = struct_def.get_fields().collect_vec();
for field in struct_fields {
let field_type = field.get_type();
let field_name = module_env
.symbol_pool()
.string(field.get_name())
.to_string();
let is_allowed = check_data_struct_fields_type(&field_type, module_env);
let is_allowed = check_data_struct_fields_type(&field_type, module_env, valid_structs);
if !is_allowed {
let struct_name = module_env
.symbol_pool()
Expand All @@ -1008,17 +1028,24 @@ fn check_data_struct_fields(struct_def: &StructEnv, module_env: &ModuleEnv) -> (
.string(struct_def.get_name())
.to_string();
let full_struct_name = format!("{}::{}", module_env.get_full_name_str(), struct_name);
valid_structs.insert(full_struct_name.clone(), true);
unsafe {
GLOBAL_DATA_STRUCT.insert(full_struct_name, true);
}

("".to_string(), true)
}

fn check_data_struct_fields_type(field_type: &Type, module_env: &ModuleEnv) -> bool {
fn check_data_struct_fields_type(
field_type: &Type,
module_env: &ModuleEnv,
valid_structs: &mut BTreeMap<String, bool>,
) -> bool {
return match field_type {
Type::Primitive(_) => true,
Type::Vector(item_type) => check_data_struct_fields_type(item_type.as_ref(), module_env),
Type::Vector(item_type) => {
check_data_struct_fields_type(item_type.as_ref(), module_env, valid_structs)
}
Type::Struct(module_id, struct_id, ty_args) => {
let module = module_env.env.get_module(*module_id);

Expand All @@ -1037,7 +1064,7 @@ fn check_data_struct_fields_type(field_type: &Type, module_env: &ModuleEnv) -> b

if is_std_option_type(&full_struct_name) {
if let Some(item_type) = ty_args.first() {
return check_data_struct_fields_type(item_type, module_env);
return check_data_struct_fields_type(item_type, module_env, valid_structs);
}

return false;
Expand All @@ -1051,7 +1078,7 @@ fn check_data_struct_fields_type(field_type: &Type, module_env: &ModuleEnv) -> b
return false;
}

check_data_struct_fields(&struct_env, &struct_module);
check_data_struct_fields(&struct_env, &struct_module, valid_structs);

let is_allowed_opt = unsafe { GLOBAL_DATA_STRUCT.get(full_struct_name.as_str()) };
return if let Some(is_allowed) = is_allowed_opt {
Expand Down Expand Up @@ -1248,13 +1275,24 @@ fn check_data_struct_func(extended_checker: &mut ExtendedChecker, module_env: &M
.entry(compiled_module.self_id())
.or_default();

let data_struct_func_map = unsafe {
let result: BTreeMap<String, Vec<usize>> = GLOBAL_DATA_STRUCT_FUNC
.iter()
.map(|(key, value)| (key.clone(), value.clone()))
.collect();
result
};
let data_struct_func_map: BTreeMap<String, Vec<usize>> = type_name_indices
.iter()
.map(|(key, value)| (key.clone(), value.clone()))
.collect();

if !data_struct_func_map.is_empty() {
log::debug!(
"\n\ncheck_data_struct_func() module {:?} data structs func start...",
module_env.get_full_name_str()
);
for (k, v) in data_struct_func_map.iter() {
log::debug!("{:?} -> {:?}", k, v);
}
log::debug!(
"\n\ncheck_data_struct_func() module {:?} data structs func end...",
module_env.get_full_name_str()
);
}

module_metadata.data_struct_func_map = data_struct_func_map;

Expand Down Expand Up @@ -1295,6 +1333,15 @@ fn check_data_struct_func(extended_checker: &mut ExtendedChecker, module_env: &M
};

if let Some(data_struct_func_indicies) = data_struct_func_types {
let caller_fun_name = fun.get_full_name_str();
log::debug!(
"function {:?}::{:?} call data_struct func {:?} with types {:?}",
module_env.self_address(),
caller_fun_name,
full_path_func_name,
data_struct_func_indicies
);

for generic_type_index in data_struct_func_indicies {
let type_arg = match type_arguments.get(generic_type_index) {
None => {
Expand Down
Loading

0 comments on commit 9d1f7b3

Please sign in to comment.