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

[verifier] Added type-checking of templated type arguments for entry functions #469

Merged
merged 4 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
27 changes: 24 additions & 3 deletions sui_programmability/adapter/src/unit_tests/adapter_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
// SPDX-License-Identifier: Apache-2.0

use crate::{adapter, genesis};
use move_binary_format::file_format::{
self, AbilitySet, AddressIdentifierIndex, IdentifierIndex, ModuleHandle, ModuleHandleIndex,
StructHandle,
use move_binary_format::{
access::ModuleAccess,
file_format::{
self, AbilitySet, AddressIdentifierIndex, IdentifierIndex, ModuleHandle, ModuleHandleIndex,
StructHandle,
},
};
use move_core_types::{account_address::AccountAddress, ident_str};
use move_package::BuildConfig;
Expand Down Expand Up @@ -924,6 +927,24 @@ fn publish_from_src(
module_path.push(src_path);
let modules = sui_framework::build_move_package(&module_path, build_config, false).unwrap();

let m = modules.get(0).unwrap().clone();

for f in &m.function_handles {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep this printing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly not! :-) Sorry it has sneaked in...

let n = m.identifier_at(f.name);
if n.to_string() == "foo" {
println!("FUNCTION {:?}", n);
println!("PARAMS");
for p in &m.signature_at(f.parameters).0 {
println!("{:?}", p);
}

println!("TYPE PARAMS");
for t in &f.type_parameters {
println!("{:?}", t);
}
}
}

// publish modules
let all_module_bytes = modules
.iter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@ module Test::M1 {
use FastX::ID::ID;
use FastX::TxContext::{Self, TxContext};
use FastX::Transfer;
use FastX::Coin::Coin;

struct Object has key, store {
id: ID,
value: u64,
}

fun foo<T: key, T2: drop>(_p1: u64, value1: T, _value2: &Coin<T2>, _p2: u64): T {
value1
}

public fun create(value: u64, recipient: vector<u8>, ctx: &mut TxContext) {
Transfer::transfer(
Object { id: TxContext::new_id(ctx), value },
Expand Down
4 changes: 3 additions & 1 deletion sui_programmability/verifier/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ name = "sui-verifier"
version = "0.1.0"
edition = "2021"
authors = ["Mysten Labs <eng@mystenlabs.com>"]
description = "Move framework for fastX platform"
description = "Move framework for Sui platform"
license = "Apache-2.0"
publish = false

[dependencies]
move-binary-format = { git = "https://github.com/diem/move", rev = "7683d09732dd930c581583bf5fde97fb7ac02ff7" }
move-bytecode-verifier = { git = "https://github.com/diem/move", rev = "7683d09732dd930c581583bf5fde97fb7ac02ff7" }
move-core-types = { git = "https://github.com/diem/move", rev = "7683d09732dd930c581583bf5fde97fb7ac02ff7", features = ["address20"] }
move-disassembler = { git = "https://github.com/diem/move", rev = "7683d09732dd930c581583bf5fde97fb7ac02ff7" }
move-ir-types = { git = "https://github.com/diem/move", rev = "7683d09732dd930c581583bf5fde97fb7ac02ff7" }

sui-types = { path = "../../sui_types" }
1 change: 1 addition & 0 deletions sui_programmability/verifier/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod verifier;
pub mod global_storage_access_verifier;
pub mod id_immutable_verifier;
pub mod id_leak_verifier;
pub mod param_typecheck_verifier;
pub mod struct_with_key_verifier;

use sui_types::error::SuiError;
Expand Down
213 changes: 213 additions & 0 deletions sui_programmability/verifier/src/param_typecheck_verifier.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
// Copyright (c) Mysten Labs
// SPDX-License-Identifier: Apache-2.0

use move_binary_format::{
binary_views::BinaryIndexedView,
file_format::{Ability, FunctionHandle, Signature, SignatureToken, Visibility},
CompiledModule,
};
use sui_types::{
base_types::{TX_CONTEXT_MODULE_NAME, TX_CONTEXT_STRUCT_NAME},
error::{SuiError, SuiResult},
SUI_FRAMEWORK_ADDRESS,
};

pub fn verify_module(module: &CompiledModule) -> SuiResult {
check_params(module)
}
/// This checks if parameters of functions that can become entry
/// functions (functions called directly from Sui) have correct types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some detailed comments explaining what exactly we are checking here would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Please let me know what you think.

pub fn check_params(module: &CompiledModule) -> SuiResult {
let view = BinaryIndexedView::Module(module);
for func_def in module.function_defs.iter() {
// find candidate entry functions and checke their parameters
// (ignore other functions)
if func_def.visibility != Visibility::Public {
// a non-public function cannot be called from Sui
continue;
}
let handle = view.function_handle_at(func_def.function);
if !view.signature_at(handle.return_).is_empty() {
// entry functions do not return values
continue;
}
let params = view.signature_at(handle.parameters);
let (candidate, param_nums) = is_entry_candidate(&view, params);
if !candidate {
continue;
}
// iterate over all object params and make sure that each
// template-typed (either by itself or in a vector) argument
// has the Key ability
for (pos, p) in params.0[0..param_nums].iter().enumerate() {
if let SignatureToken::TypeParameter(_) = p {
if !is_template_param_ok(handle, p) {
return Err(SuiError::ModuleVerificationFailure {
error: format!(
"No Key ability for the template parameter at position {} in entry function {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"No Key ability for the template parameter at position {} in entry function {}",
"No `key` ability for the template parameter at position {} in entry function {}",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

pos,
view.identifier_at(handle.name)
),
});
}
}
if let SignatureToken::Vector(_) = p {
if !is_template_vector_param_ok(handle, p) {
return Err(SuiError::ModuleVerificationFailure {
error: format!(
"No Key ability for the template vector parameter at position {} in entry function {}",
pos,
view.identifier_at(handle.name)
),
});
}
}
}
}
Ok(())
}

fn is_entry_candidate(view: &BinaryIndexedView, params: &Signature) -> (bool, usize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the usize return value is only needed when the first bool is true, then this is perfect case to return Option<usize>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wearing my Go hat I guess... Thank you!

let mut obj_params_num = 0;
if params.is_empty() {
// must have at least on &mut TxContext param
return (false, obj_params_num);
}
let last_param = params.0.get(params.len() - 1).unwrap();
if !is_tx_context(view, last_param) {
return (false, obj_params_num);
}
if params.len() == 1 {
// only one &mut TxContext param
return (true, obj_params_num);
}
// currently, an entry function has object parameters followed by
// ground-type parameters (followed by the &mut TxContext param,
// but we already checked this one)
let mut ground_params_phase = false; // becomes true once we start seeing ground-type params
for p in &params.0[0..params.len() - 1] {
if is_ground_type(p) {
ground_params_phase = true;
} else {
obj_params_num += 1;
if ground_params_phase {
// We encounter a non ground-type parameter after the
// first one was encountered. This cannot be an entry
// function as it would get rejected by the
// resolve_and_type_check function in the adapter upon its
// call attempt from Sui
return (false, obj_params_num);
Copy link
Contributor

@lxfind lxfind Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the semantics of is_entry_candidate? Is it to detect whether this function could be an entry function, or is it to detect whether this could be an entry function that has the potential to violate integrity?

Consider this one:

public fun foo<T>(a: u64, b: T, _ctx: &mut TxContext)

If foo is called with T being a primitive type, it can certainly be an entry function, but it will fail here and return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It checks if the function can be an entry candidate without checking correctness of parameter types (added a comment to that effect).

The example you give is almost the exact one I mentioned in my comment - under current implementation this function would not be considered an entry function candidate, much like it would throw an validation error (I think) upon being called from Sui.

}
if !is_object(view, p)
&& !is_template(p)
&& !is_object_vector(view, p)
&& !is_template_vector(p)
{
// A non-ground type for entry functions must be an
// object, or a generic type, or a vector (possibly
// nested) of objects, or a templeted vector (possibly
// nested). Otherwise it is not an entry function as
// we cannot pass non-object types from Sui).
return (false, obj_params_num);
}
}
}
(true, obj_params_num)
}

/// Checks if a given parameter is of ground type. It's a mirror of
/// the is_primitive function in the adapter module that operates on
/// Type-s.
fn is_ground_type(p: &SignatureToken) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we call it ground instead of primitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, other than @sblackshear called them as such in one of our Slack conversations and I thought that's what I should use :-)

use SignatureToken::*;
match p {
Bool | U8 | U64 | U128 | Address => true,
Vector(t) => is_ground_type(t),
Signer
| Struct(_)
| StructInstantiation(..)
| Reference(_)
| MutableReference(_)
| TypeParameter(_) => false,
}
}

fn is_object(view: &BinaryIndexedView, p: &SignatureToken) -> bool {
use SignatureToken::*;
match p {
Struct(idx) => view
.struct_handle_at(*idx)
.abilities
.has_ability(Ability::Key),
StructInstantiation(idx, _) => view
.struct_handle_at(*idx)
.abilities
.has_ability(Ability::Key),
Reference(t) => is_object(view, t),
MutableReference(t) => is_object(view, t),
_ => false,
}
}

fn is_template(p: &SignatureToken) -> bool {
matches!(p, SignatureToken::TypeParameter(_))
}

fn is_object_vector(view: &BinaryIndexedView, p: &SignatureToken) -> bool {
if let SignatureToken::Vector(t) = p {
match &**t {
SignatureToken::Vector(inner_t) => return is_object_vector(view, inner_t),
other => return is_object(view, other),
}
}
false
}

fn is_template_vector(p: &SignatureToken) -> bool {
match p {
SignatureToken::Vector(t) => is_template_vector(t),
other => matches!(other, SignatureToken::TypeParameter(_)),
}
}

fn is_template_param_ok(handle: &FunctionHandle, p: &SignatureToken) -> bool {
if let SignatureToken::TypeParameter(idx) = p {
if !handle
.type_parameters
.get(*idx as usize)
.unwrap()
.has_ability(Ability::Key)
{
return false;
}
}
true
}

fn is_template_vector_param_ok(handle: &FunctionHandle, p: &SignatureToken) -> bool {
match p {
SignatureToken::Vector(t) => is_template_vector_param_ok(handle, t),
SignatureToken::TypeParameter(_) => is_template_param_ok(handle, p),
_ => true,
}
}

fn is_tx_context(view: &BinaryIndexedView, p: &SignatureToken) -> bool {
match p {
SignatureToken::MutableReference(m) => match &**m {
SignatureToken::Struct(idx) => {
let struct_handle = view.struct_handle_at(*idx);
let struct_name = view.identifier_at(struct_handle.name);
let module = view.module_handle_at(struct_handle.module);
let module_name = view.identifier_at(module.name);
let module_addr = view.address_identifier_at(module.address);
module_name == TX_CONTEXT_MODULE_NAME
&& module_addr == &SUI_FRAMEWORK_ADDRESS
&& struct_name == TX_CONTEXT_STRUCT_NAME
&& struct_handle.type_parameters.is_empty()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type param check is not necessary I think. There is only one unique struct given address, module and struct name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function mirrors is_param_tx_context in adapter which has the same checks (but on the Type enum rather than SignatureToken).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be removed there too. On a side note, we are starting to have more duplicate code between adapter and verifier. Latter we probably should move some of the common code to a separate place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

}
_ => false,
},
_ => false,
}
}
5 changes: 3 additions & 2 deletions sui_programmability/verifier/src/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ use sui_types::error::SuiResult;

use crate::{
global_storage_access_verifier, id_immutable_verifier, id_leak_verifier,
struct_with_key_verifier,
param_typecheck_verifier, struct_with_key_verifier,
};

/// Helper for a "canonical" verification of a module.
pub fn verify_module(module: &CompiledModule) -> SuiResult {
struct_with_key_verifier::verify_module(module)?;
global_storage_access_verifier::verify_module(module)?;
id_immutable_verifier::verify_module(module)?;
id_leak_verifier::verify_module(module)
id_leak_verifier::verify_module(module)?;
param_typecheck_verifier::verify_module(module)
}
Loading