Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[move] Type builder & fix calculation of node count in types #13028

Merged
merged 32 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
c046a17
add num nodes check
georgemitenkov Apr 25, 2024
8088c12
propagate error in txn arg validation
georgemitenkov Apr 25, 2024
eb00ec4
unhappy tests
georgemitenkov Apr 25, 2024
cd2608f
add type config
georgemitenkov Apr 28, 2024
404efb1
constant types to use builder
georgemitenkov Apr 28, 2024
05c701c
type substitution to use builder
georgemitenkov Apr 29, 2024
7cfc04f
type loading to use builder
georgemitenkov Apr 29, 2024
78d68ae
Merge branch 'main' into george/types
georgemitenkov Apr 29, 2024
a55fa89
encapsulate paranoid checks (pt 1)
georgemitenkov Apr 29, 2024
99f67a7
remove useless results
georgemitenkov Apr 29, 2024
47ec582
encapsulate paranoid checks (pt 2)
georgemitenkov Apr 29, 2024
48571a6
minor API refactoring
georgemitenkov Apr 30, 2024
cb81a44
tests + better error messages
georgemitenkov Apr 30, 2024
a9455c3
Merge branch 'main' into george/types
georgemitenkov May 6, 2024
a9ffbce
Merge branch 'main' into george/types
georgemitenkov May 10, 2024
1d0f8be
Merge branch 'main' into george/types
georgemitenkov May 31, 2024
afb02c3
remove type_size_limit because it is always true
georgemitenkov May 31, 2024
374af56
remove production type config
georgemitenkov May 31, 2024
f63efef
feature gating type builder creations by using versioned builder
georgemitenkov May 31, 2024
a003d04
Merge branch 'main' into george/types
georgemitenkov May 31, 2024
e3d03ec
Merge branch 'main' into george/types
georgemitenkov Jun 3, 2024
f2e0d57
unit testing form type creation pt 1
georgemitenkov Jun 4, 2024
aaaa279
finish feature gating
georgemitenkov Jun 4, 2024
da4b97e
address comments and make gas params work
georgemitenkov Jun 11, 2024
8647869
Merge branch 'main' into george/types
georgemitenkov Jun 11, 2024
275704e
make type size limit non default feature
georgemitenkov Jun 11, 2024
034d5f2
multiple fixes & improvements:
georgemitenkov Jun 11, 2024
2569a0d
fix releas to 1.15
georgemitenkov Jun 11, 2024
9e996e9
avoid cloning
georgemitenkov Jun 11, 2024
f89a2e5
set limits to 128 and 20
georgemitenkov Jun 12, 2024
5840e2b
Merge branch 'main' into george/types
georgemitenkov Jun 12, 2024
1f99379
Merge branch 'main' into george/types
georgemitenkov Jun 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,10 @@
let allowed_structs = get_allowed_structs(are_struct_constructors_enabled);
// Need to keep this here to ensure we return the historic correct error code for replay
for ty in func.parameters[signer_param_cnt..].iter() {
let valid = is_valid_txn_arg(
session,
&ty.subst(&func.type_arguments).unwrap(),
allowed_structs,
);
let ty = ty
.subst(&func.type_arguments)
.map_err(|e| e.finish(Location::Undefined).into_vm_status())?;
let valid = is_valid_txn_arg(session, &ty, allowed_structs);

Check warning on line 135 in aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs#L132-L135

Added lines #L132 - L135 were not covered by tests
if !valid {
return Err(VMStatus::error(
StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE,
Expand Down Expand Up @@ -224,14 +223,10 @@
return Err(invalid_signature());
}
for (ty, arg) in types.iter().zip(args) {
let arg = construct_arg(
session,
&ty.subst(ty_args).unwrap(),
allowed_structs,
arg,
&mut gas_meter,
is_view,
)?;
let ty = ty
.subst(ty_args)
.map_err(|e| e.finish(Location::Undefined).into_vm_status())?;
let arg = construct_arg(session, &ty, allowed_structs, arg, &mut gas_meter, is_view)?;

Check warning on line 229 in aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs#L226-L229

Added lines #L226 - L229 were not covered by tests
res_args.push(arg);
}
Ok(res_args)
Expand Down
23 changes: 8 additions & 15 deletions third_party/move/move-vm/runtime/src/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl Interpreter {
}

let mut current_frame = self
.make_new_frame(gas_meter, loader, module_store, function, ty_args, locals)
.make_new_frame(gas_meter, function, ty_args, locals)
.map_err(|err| self.set_location(err))?;
// Access control for the new frame.
self.access_control
Expand Down Expand Up @@ -223,7 +223,7 @@ impl Interpreter {
continue;
}
let frame = self
.make_call_frame(gas_meter, loader, module_store, func, vec![])
.make_call_frame(gas_meter, loader, func, vec![])
.map_err(|err| {
self.attach_state_if_invariant_violation(
self.set_location(err),
Expand Down Expand Up @@ -284,7 +284,7 @@ impl Interpreter {
continue;
}
let frame = self
.make_call_frame(gas_meter, loader, module_store, func, ty_args)
.make_call_frame(gas_meter, loader, func, ty_args)
.map_err(|err| {
self.attach_state_if_invariant_violation(
self.set_location(err),
Expand Down Expand Up @@ -317,7 +317,6 @@ impl Interpreter {
&mut self,
gas_meter: &mut impl GasMeter,
loader: &Loader,
module_store: &ModuleStorageAdapter,
func: Arc<Function>,
ty_args: Vec<Type>,
) -> PartialVMResult<Frame> {
Expand All @@ -336,18 +335,15 @@ impl Interpreter {

if self.paranoid_type_checks {
let ty = self.operand_stack.pop_ty()?;
let resolver = func.get_resolver(loader, module_store);
if is_generic {
ty.check_eq(
&resolver.subst(&func.local_types()[arg_count - i - 1], &ty_args)?,
)?;
ty.check_eq(&func.local_types()[arg_count - i - 1].subst(&ty_args)?)?;
} else {
// Directly check against the expected type to save a clone here.
ty.check_eq(&func.local_types()[arg_count - i - 1])?;
}
}
}
self.make_new_frame(gas_meter, loader, module_store, func, ty_args, locals)
self.make_new_frame(gas_meter, func, ty_args, locals)
}

/// Create a new `Frame` given a `Function` and the function `Locals`.
Expand All @@ -356,8 +352,6 @@ impl Interpreter {
fn make_new_frame(
&self,
gas_meter: &mut impl GasMeter,
loader: &Loader,
module_store: &ModuleStorageAdapter,
function: Arc<Function>,
ty_args: Vec<Type>,
locals: Locals,
Expand All @@ -371,11 +365,10 @@ impl Interpreter {
if ty_args.is_empty() {
function.local_types().to_vec()
} else {
let resolver = function.get_resolver(loader, module_store);
function
.local_types()
.iter()
.map(|ty| resolver.subst(ty, &ty_args))
.map(|ty| ty.subst(&ty_args))
.collect::<PartialVMResult<Vec<_>>>()?
}
} else {
Expand Down Expand Up @@ -447,7 +440,7 @@ impl Interpreter {
if self.paranoid_type_checks {
for i in 0..expected_args {
let expected_ty =
resolver.subst(&function.parameter_types()[expected_args - i - 1], &ty_args)?;
function.parameter_types()[expected_args - i - 1].subst(&ty_args)?;
let ty = self.operand_stack.pop_ty()?;
ty.check_eq(&expected_ty)?;
}
Expand Down Expand Up @@ -517,7 +510,7 @@ impl Interpreter {

if self.paranoid_type_checks {
for ty in function.return_types() {
self.operand_stack.push_ty(resolver.subst(ty, &ty_args)?)?;
self.operand_stack.push_ty(ty.subst(&ty_args)?)?;
}
}
Ok(())
Expand Down
137 changes: 5 additions & 132 deletions third_party/move/move-vm/runtime/src/loader/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,21 +321,6 @@ impl Loader {
type_arguments.push(self.load_type(ty, data_store, module_store)?);
}

if self.vm_config.type_size_limit
&& type_arguments
.iter()
.map(|loaded_ty| self.count_type_nodes(loaded_ty))
.sum::<u64>()
> MAX_TYPE_INSTANTIATION_NODES
{
return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES)
.with_message(format!(
"Too many type nodes when instantiating a type for script {}",
&main.name
))
.finish(Location::Script));
};

self.verify_ty_args(main.type_parameters(), &type_arguments)
.map_err(|e| {
e.with_message(format!(
Expand Down Expand Up @@ -1232,49 +1217,6 @@ impl Loader {
Ok(())
}

// Return an instantiated type given a generic and an instantiation.
// Stopgap to avoid a recursion that is either taking too long or using too
// much memory
fn subst(&self, ty: &Type, ty_args: &[Type]) -> PartialVMResult<Type> {
// Before instantiating the type, count the # of nodes of all type arguments plus
// existing type instantiation.
// If that number is larger than MAX_TYPE_INSTANTIATION_NODES, refuse to construct this type.
// This prevents constructing larger and lager types via struct instantiation.
match ty {
Type::MutableReference(_) | Type::Reference(_) | Type::Vector(_) => {
if self.vm_config.type_size_limit
&& self.count_type_nodes(ty) > MAX_TYPE_INSTANTIATION_NODES
{
return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES));
}
},
Type::StructInstantiation {
ty_args: struct_inst,
..
} => {
let mut sum_nodes = 1u64;
for ty in ty_args.iter().chain(struct_inst.iter()) {
sum_nodes = sum_nodes.saturating_add(self.count_type_nodes(ty));
if sum_nodes > MAX_TYPE_INSTANTIATION_NODES {
return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES));
}
}
},
Type::Address
| Type::Bool
| Type::Signer
| Type::Struct { .. }
| Type::TyParam(_)
| Type::U8
| Type::U16
| Type::U32
| Type::U64
| Type::U128
| Type::U256 => (),
};
ty.subst(ty_args)
}

// Verify the kind (constraints) of an instantiation.
// Both function and script invocation use this function to verify correctness
// of type arguments provided
Expand Down Expand Up @@ -1415,29 +1357,11 @@ impl<'a> Resolver<'a> {

let mut instantiation = vec![];
for ty in &func_inst.instantiation {
instantiation.push(self.subst(ty, ty_args)?);
}
// Check if the function instantiation over all generics is larger
// than MAX_TYPE_INSTANTIATION_NODES.
let mut sum_nodes = 1u64;
for ty in ty_args.iter().chain(instantiation.iter()) {
sum_nodes = sum_nodes.saturating_add(self.loader.count_type_nodes(ty));
if sum_nodes > MAX_TYPE_INSTANTIATION_NODES {
return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES));
}
instantiation.push(ty.subst(ty_args)?);
}
Ok(instantiation)
}

#[allow(unused)]
pub(crate) fn type_params_count(&self, idx: FunctionInstantiationIndex) -> usize {
let func_inst = match &self.binary {
BinaryType::Module(module) => module.function_instantiation_at(idx.0),
BinaryType::Script(script) => script.function_instantiation_at(idx.0),
};
func_inst.instantiation.len()
}

//
// Type resolution
//
Expand All @@ -1463,31 +1387,14 @@ impl<'a> Resolver<'a> {
BinaryType::Script(_) => unreachable!("Scripts cannot have type instructions"),
};

// Before instantiating the type, count the # of nodes of all type arguments plus
// existing type instantiation.
// If that number is larger than MAX_TYPE_INSTANTIATION_NODES, refuse to construct this type.
// This prevents constructing larger and larger types via struct instantiation.
let mut sum_nodes = 1u64;
for ty in ty_args.iter().chain(struct_inst.instantiation.iter()) {
sum_nodes = sum_nodes.saturating_add(self.loader.count_type_nodes(ty));
if sum_nodes > MAX_TYPE_INSTANTIATION_NODES {
return Err(
PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES).with_message(format!(
"Number of type instantiation nodes exceeded the maximum of {}",
MAX_TYPE_INSTANTIATION_NODES
)),
);
}
}

let struct_ = &struct_inst.definition_struct_type;
Ok(Type::StructInstantiation {
idx: struct_.idx,
ty_args: triomphe::Arc::new(
struct_inst
.instantiation
.iter()
.map(|ty| self.subst(ty, ty_args))
.map(|ty| ty.subst(ty_args))
.collect::<PartialVMResult<_>>()?,
),
ability: AbilityInfo::generic_struct(
Expand Down Expand Up @@ -1524,7 +1431,6 @@ impl<'a> Resolver<'a> {
.map(|inst_ty| inst_ty.subst(ty_args))
.collect::<PartialVMResult<Vec<_>>>()?;

// TODO: Is this type substitution unbounded?
field_instantiation.definition_struct_type.fields[field_instantiation.offset]
.subst(&instantiation_types)
}
Expand Down Expand Up @@ -1578,16 +1484,12 @@ impl<'a> Resolver<'a> {
let ty = self.single_type_at(idx);

if !ty_args.is_empty() {
self.subst(ty, ty_args)
ty.subst(ty_args)
} else {
Ok(ty.clone())
}
}

pub(crate) fn subst(&self, ty: &Type, ty_args: &[Type]) -> PartialVMResult<Type> {
self.loader.subst(ty, ty_args)
}

//
// Fields resolution
//
Expand Down Expand Up @@ -1745,10 +1647,6 @@ pub const VALUE_DEPTH_MAX: u64 = 128;
/// fields for struct types.
const MAX_TYPE_TO_LAYOUT_NODES: u64 = 256;

/// Maximal nodes which are all allowed when instantiating a generic type. This does not include
/// field types of structs.
const MAX_TYPE_INSTANTIATION_NODES: u64 = 128;

struct PseudoGasContext {
max_cost: u64,
cost: u64,
Expand Down Expand Up @@ -1851,31 +1749,6 @@ impl Loader {
})
}

fn count_type_nodes(&self, ty: &Type) -> u64 {
let mut todo = vec![ty];
let mut result = 0;
while let Some(ty) = todo.pop() {
match ty {
Type::Vector(ty) => {
result += 1;
todo.push(ty);
},
Type::Reference(ty) | Type::MutableReference(ty) => {
result += 1;
todo.push(ty);
},
Type::StructInstantiation { ty_args, .. } => {
result += 1;
todo.extend(ty_args.iter())
},
_ => {
result += 1;
},
}
}
result
}

fn struct_name_to_type_layout(
&self,
struct_idx: StructNameIndex,
Expand Down Expand Up @@ -1907,7 +1780,7 @@ impl Loader {
let field_tys = struct_type
.fields
.iter()
.map(|ty| self.subst(ty, ty_args))
.map(|ty| ty.subst(ty_args))
.collect::<PartialVMResult<Vec<_>>>()?;
let (mut field_layouts, field_has_identifier_mappings): (Vec<MoveTypeLayout>, Vec<bool>) =
field_tys
Expand Down Expand Up @@ -2119,7 +1992,7 @@ impl Loader {
.iter()
.zip(&struct_type.fields)
.map(|(n, ty)| {
let ty = self.subst(ty, ty_args)?;
let ty = ty.subst(ty_args)?;
let l =
self.type_to_fully_annotated_layout_impl(&ty, module_store, count, depth)?;
Ok(MoveFieldLayout::new(n.clone(), l))
Expand Down
1 change: 0 additions & 1 deletion third_party/move/move-vm/runtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,6 @@ impl VMRuntime {
.collect()
}

#[allow(clippy::needless_collect)]
fn execute_function_impl(
&self,
func: Arc<Function>,
Expand Down
Loading
Loading