Skip to content

Commit

Permalink
[Compiler-v2] check access for structs (#12821)
Browse files Browse the repository at this point in the history
* check field

* handle comments
  • Loading branch information
rahxephon89 authored Apr 10, 2024
1 parent be0ef97 commit 59586fe
Show file tree
Hide file tree
Showing 26 changed files with 690 additions and 159 deletions.
124 changes: 123 additions & 1 deletion third_party/move/move-compiler-v2/src/function_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use crate::Options;
use codespan_reporting::diagnostic::Severity;
use move_binary_format::file_format::Visibility;
use move_model::{
model::{FunId, FunctionEnv, GlobalEnv, Loc, NodeId, QualifiedId},
ast::{ExpData, Operation, Pattern},
model::{FunId, FunctionEnv, GlobalEnv, Loc, ModuleEnv, NodeId, QualifiedId},
ty::Type,
};
use std::{collections::BTreeSet, iter::Iterator, vec::Vec};
Expand Down Expand Up @@ -56,13 +57,131 @@ pub fn check_for_function_typed_parameters(env: &mut GlobalEnv) {
}
}

fn access_error(
env: &GlobalEnv,
fun_loc: &Loc,
id: &NodeId,
oper: &str,
msg: String,
module_env: &ModuleEnv,
) {
let call_details: Vec<_> = [*id]
.iter()
.map(|node_id| (env.get_node_loc(*node_id), format!("{} here", oper)))
.collect();
let msg = format!(
"Invalid operation: {} can only be done within the defining module `{}`",
msg,
module_env.get_full_name_str()
);
env.diag_with_labels(Severity::Error, fun_loc, &msg, call_details);
}

/// check privileged operations on a struct such as storage operation, pack/unpack and field accesses
/// can only be performed within the module that defines it.
fn check_privileged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
if let Some(fun_body) = fun_env.get_def() {
let caller_module_id = fun_env.module_env.get_id();
fun_body.visit_pre_order(&mut |exp: &ExpData| {
match exp {
ExpData::Call(id, oper, _) => match oper {
Operation::Exists(_)
| Operation::BorrowGlobal(_)
| Operation::MoveFrom
| Operation::MoveTo => {
let inst = env.get_node_instantiation(*id);
debug_assert!(!inst.is_empty());
if let Some((struct_env, _)) = inst[0].get_struct(env) {
let mid = struct_env.module_env.get_id();
let sid = struct_env.get_id();
if mid != caller_module_id {
let qualified_struct_id = mid.qualified(sid);
let struct_env = env.get_struct(qualified_struct_id);
access_error(
env,
&fun_env.get_id_loc(),
id,
"called",
format!(
"storage operation on type `{}`",
struct_env.get_full_name_str(),
),
&struct_env.module_env,
);
}
}
},
Operation::Select(mid, sid, fid) if *mid != caller_module_id => {
let qualified_struct_id = mid.qualified(*sid);
let struct_env = env.get_struct(qualified_struct_id);
access_error(
env,
&fun_env.get_id_loc(),
id,
"accessed",
format!(
"access of the field `{}` on type `{}`",
fid.symbol().display(struct_env.symbol_pool()),
struct_env.get_full_name_str(),
),
&struct_env.module_env,
);
},
Operation::Pack(mid, sid) => {
if *mid != caller_module_id {
let qualified_struct_id = mid.qualified(*sid);
let struct_env = env.get_struct(qualified_struct_id);
access_error(
env,
&fun_env.get_id_loc(),
id,
"packed",
format!("pack of `{}`", struct_env.get_full_name_str(),),
&struct_env.module_env,
);
}
},
_ => {},
},
ExpData::Assign(_, pat, _)
| ExpData::Block(_, pat, _, _)
| ExpData::Lambda(_, pat, _) => {
pat.visit_pre_post(&mut |_, pat| {
if let Pattern::Struct(id, str, _) = pat {
let module_id = str.module_id;
if module_id != caller_module_id {
let struct_env = env.get_struct(str.to_qualified_id());
access_error(
env,
&fun_env.get_id_loc(),
id,
"unpacked",
format!("unpack of `{}`", struct_env.get_full_name_str(),),
&struct_env.module_env,
);
}
}
});
},
// access in specs is not restricted
ExpData::SpecBlock(_, _) => {
return false;
},
_ => {},
}
true
});
}
}

/// For all function in target modules:
///
/// If `before_inlining`, then
/// - check that all function calls involving inline functions are accessible;
/// - warn about unused private functions
/// Otherwise (`!before_inlining`):
/// - check that all function calls *not* involving inline functions are accessible.
/// - check privileged operations on structs cannot be done across module boundary
pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
// For each function seen, we record whether it has an accessible caller.
let mut functions_with_callers: BTreeSet<QualifiedFunId> = BTreeSet::new();
Expand All @@ -77,6 +196,9 @@ pub fn check_access_and_use(env: &mut GlobalEnv, before_inlining: bool) {
let caller_module_has_friends = !caller_module.has_no_friends();
let caller_module_is_script = caller_module.get_name().is_script();
for caller_func in caller_module.get_functions() {
if !before_inlining {
check_privileged_operations_on_structs(env, &caller_func);
}
let caller_qfid = caller_func.get_qualified_id();

// During first pass, record private functions for later
Expand Down
Original file line number Diff line number Diff line change
@@ -1,39 +1,39 @@

Diagnostics:
error: invalid access specifier
┌─ tests/checking-lang-v1/invalid_type_acquire.move:18:9
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:18:9
18 │ T,
│ ^

error: not supported before language version `2.0-unstable`: address and wildcard access specifiers. Only resource type names can be provided.
┌─ tests/checking-lang-v1/invalid_type_acquire.move:18:9
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:18:9
18 │ T,
│ ^

error: invalid access specifier
┌─ tests/checking-lang-v1/invalid_type_acquire.move:19:9
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:19:9
19 │ u64,
│ ^^^

error: not supported before language version `2.0-unstable`: address and wildcard access specifiers. Only resource type names can be provided.
┌─ tests/checking-lang-v1/invalid_type_acquire.move:19:9
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:19:9
19 │ u64,
│ ^^^

error: type `u64` is missing required ability `key`
┌─ tests/checking-lang-v1/invalid_type_acquire.move:32:36
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:32:36
32 │ destroy(account, move_from<u64>(a));
│ ^^^
= required by instantiating type parameter `T:key` of function `move_from`

error: type `S` is missing required ability `key`
┌─ tests/checking-lang-v1/invalid_type_acquire.move:34:36
┌─ tests/checking-lang-v1/v1-typing/invalid_type_acquire.move:34:36
34 │ destroy(account, move_from<S>(a));
│ ^
Expand Down
Original file line number Diff line number Diff line change
@@ -1,28 +1,12 @@
// -- Model dump before bytecode pipeline
module 0x42::objects {
struct ReaderRef {
addr: address,
}
} // end 0x42::objects
module 0x42::token {
use 0x42::objects as obj; // resolved as: 0x42::objects
struct Token {
val: u64,
}
public fun get_value(ref: &objects::ReaderRef<token::Token>): u64
acquires token::Token(*)
{
select token::Token.val<&token::Token>({
let (ref: &objects::ReaderRef<token::Token>): (&objects::ReaderRef<token::Token>) = Tuple(ref);
BorrowGlobal(Immutable)<token::Token>(select objects::ReaderRef.addr<&objects::ReaderRef<token::Token>>(ref))
})
}
} // end 0x42::token


Diagnostics:
bug: struct not defined
error: Invalid operation: access of the field `addr` on type `objects::ReaderRef` can only be done within the defining module `0x42::objects`
┌─ tests/checking/inlining/resources_invalid.move:17:16
8 │ borrow_global<T>(ref.addr)
│ -------- accessed here
·
17 │ public fun get_value(ref: &obj::ReaderRef<Token>): u64 acquires Token {
│ ^^^^^^^^^
18 │ obj::reader(ref).val
│ ---------------- from a call inlined at this callsite
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
// -- Model dump before bytecode pipeline
module 0x2::X {
struct S {
f: u64,
}
public fun s(): X::S {
pack X::S(0)
}
} // end 0x2::X
module 0x2::M {
use 0x2::X; // resolved as: 0x2::X
private fun t0() {
Borrow(Immutable)(select X::S.f<X::S>(X::s()));
{
let s: &X::S = Borrow(Immutable)(X::s());
Borrow(Immutable)(select X::S.f<&X::S>(s));
Abort(0)
}
}
} // end 0x2::M

Diagnostics:
error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/borrow_field_internal.move:12:9
12 │ fun t0() {
│ ^^
13 │ (&X::s().f: &u64);
│ -------- accessed here

error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/borrow_field_internal.move:12:9
12 │ fun t0() {
│ ^^
·
15 │ (&s.f: &u64);
│ --- accessed here
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
// -- Model dump before bytecode pipeline
module 0x42::M {
struct R {
dummy_field: bool,
}
public fun new(): M::R {
pack M::R(false)
}
} // end 0x42::M
module <SELF>_0 {
use 0x42::M; // resolved as: 0x42::M
private fun test<Token>(account: signer) {
{
let r: M::R = M::new();
BorrowGlobal(Immutable)<M::R>(0x1);
MoveTo<M::R>(Borrow(Immutable)(account), r);
Tuple()
}
}
} // end <SELF>_0

Diagnostics:
error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
┌─ tests/checking/typing/global_builtins_script.move:14:5
14 │ fun test<Token>(account: signer) {
│ ^^^^
15 │ let r = M::new();
16 │ borrow_global<M::R>(@0x1);
│ ------------------------- called here

error: Invalid operation: storage operation on type `M::R` can only be done within the defining module `0x42::M`
┌─ tests/checking/typing/global_builtins_script.move:14:5
14 │ fun test<Token>(account: signer) {
│ ^^^^
·
17 │ move_to(&account, r);
│ -------------------- called here
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
// -- Model dump before bytecode pipeline
module 0x2::X {
struct S {
f: u64,
}
public fun s(): X::S {
pack X::S(0)
}
} // end 0x2::X
module 0x2::M {
use 0x2::X; // resolved as: 0x2::X
private fun t0() {
select X::S.f<X::S>(X::s());
{
let s: &X::S = Borrow(Immutable)(X::s());
select X::S.f<&X::S>(s);
Abort(0)
}
}
} // end 0x2::M

Diagnostics:
error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/implicit_deref_borrow_field_internal.move:12:9
12 │ fun t0() {
│ ^^
13 │ (X::s().f: u64);
│ -------- accessed here

error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/implicit_deref_borrow_field_internal.move:12:9
12 │ fun t0() {
│ ^^
·
15 │ (s.f: u64);
│ --- accessed here
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
// -- Model dump before bytecode pipeline
module 0x2::X {
struct S {
f: u64,
}
public fun s(): X::S {
pack X::S(0)
}
} // end 0x2::X
module 0x2::M {
use 0x2::X; // resolved as: 0x2::X
private fun t0() {
select X::S.f<X::S>(X::s()) = 0;
{
let s: &mut X::S = Borrow(Mutable)(X::s());
select X::S.f<&mut X::S>(s) = 0;
Abort(0)
}
}
} // end 0x2::M

Diagnostics:
error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/mutate_field_internal.move:12:9
12 │ fun t0() {
│ ^^
13 │ X::s().f = 0;
│ -------- accessed here

error: Invalid operation: access of the field `f` on type `X::S` can only be done within the defining module `0x2::X`
┌─ tests/checking/typing/mutate_field_internal.move:12:9
12 │ fun t0() {
│ ^^
·
15 │ s.f = 0;
│ --- accessed here
3 changes: 2 additions & 1 deletion third_party/move/move-compiler-v2/tests/testsuite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,8 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
include: vec!["/acquires-checker/"],
exclude: vec![],
exp_suffix: None,
options: opts.clone(),
// Skip access check to avoid error message change in the acquires-checker
options: opts.clone().set_experiment(Experiment::ACCESS_CHECK, false),
// Run the full compiler pipeline to double-check the result.
stop_after: StopAfter::FileFormat,
dump_ast: DumpLevel::None,
Expand Down
Loading

0 comments on commit 59586fe

Please sign in to comment.