[Compiler-v2] check access for structs

third_party/move/move-compiler-v2/src/function_checker.rs

@@ -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};
@@ -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();
@@ -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

third_party/move/move-compiler-v2/tests/checking-lang-v1/invalid_type_acquire.exp → third_party/move/move-compiler-v2/tests/checking-lang-v1/v1-typing/invalid_type_acquire.exp

@@ -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));
    │                                    ^

third_party/move/move-compiler-v2/tests/checking/inlining/resources_invalid.exp

@@ -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

third_party/move/move-compiler-v2/tests/checking/typing/borrow_field_internal.exp

@@ -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

third_party/move/move-compiler-v2/tests/checking/typing/global_builtins_script.exp

@@ -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

third_party/move/move-compiler-v2/tests/checking/typing/implicit_deref_borrow_field_internal.exp

@@ -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

third_party/move/move-compiler-v2/tests/checking/typing/mutate_field_internal.exp

@@ -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

third_party/move/move-compiler-v2/tests/testsuite.rs

@@ -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,