[Compiler-v2] check access for structs #12821

rahxephon89 · 2024-04-08T04:48:38Z

Description

This PR adds the check to guarantee a struct can only be accessed within the module that defines it.

Close #11014
Close #12820

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Existing tests;
Migrated three V1 tests;
New tests

Key Areas to Review

Whether all ways of accessing a struct are covered.

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-04-08T04:48:42Z

⏱️ 13h 35m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	2h 36m	🟥 🟥 🟥 🟥 🟥 (+3 more)
rust-unit-tests	2h	🟥 🟩 🟩 ⬜ 🟩 (+1 more)
rust-targeted-unit-tests	1h 31m	🟩 🟩 ⬜ 🟩 🟩
rust-move-tests	1h 22m	🟩 🟩 ⬜ 🟩 🟩 (+4 more)
rust-lints	53m	🟩 🟩 ⬜ 🟩 🟩 (+4 more)
execution-performance / single-node-performance	47m	🟩 🟩
run-tests-main-branch	36m	🟩 🟩 🟩 🟩 🟩 (+3 more)
windows-build	34m	🟩 🟩
rust-smoke-tests	31m	⬜ 🟩
forge-e2e-test / forge	31m	🟩 🟩
rust-images / rust-all	26m	🟩 🟩
forge-compat-test / forge	23m	🟩 🟩
cli-e2e-tests / run-cli-tests	23m	🟩 🟩
general-lints	18m	🟩 🟩 ⬜ 🟩 🟩 (+4 more)
check-dynamic-deps	18m	🟩 🟩 🟩 🟩 🟩 (+4 more)
check	6m	⬜ 🟩
rust-build-cached-packages	6m	⬜ 🟩
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 ⬜ 🟩 🟩 (+4 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
permission-check	30s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	29s	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	27s	🟩 🟩
permission-check	27s	🟩 🟩 🟩 🟩 🟩 (+4 more)
execution-performance / file_change_determinator	25s	🟩 🟩
permission-check	22s	🟩 🟩 🟩 🟩 🟩 (+4 more)
determine-docker-build-metadata	10s	🟩 🟩
permission-check	6s	🟩 🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-targeted-unit-tests	20m	7m
windows-build	15m	19m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

rahxephon89 · 2024-04-08T04:50:10Z

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

@@ -335,7 +335,7 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
            include: vec!["/acquires-checker/"],
            exclude: vec![],
            exp_suffix: None,
-            options: opts.clone(),
+            options: opts.clone().set_experiment(Experiment::ACCESS_CHECK, false),


skip access check for acquires-checker so that the error messages remain the same.

Could you add this as a comment please?

rahxephon89 · 2024-04-08T04:51:00Z

third_party/move/move-compiler-v2/tests/v1.matched

@@ -430,34 +433,3 @@ move-compiler/tests/move_check/typing/while_body.exp   move-compiler-v2/tests/ch
 move-compiler/tests/move_check/typing/while_body_invalid.exp   move-compiler-v2/tests/checking/typing/while_body_invalid.exp
 move-compiler/tests/move_check/typing/while_condition.exp   move-compiler-v2/tests/checking/typing/while_condition.exp
 move-compiler/tests/move_check/typing/while_condition_invalid.exp   move-compiler-v2/tests/checking/typing/while_condition_invalid.exp
-move-compiler/tests/move_check/parser/aptos_stdlib_attributes.exp   move-compiler-v2/tests/checking/attributes/aptos_stdlib_attributes.exp


After Brian's PR to fix the update tool is landed, these will be recovered.

rahxephon89 · 2024-04-08T04:51:14Z

third_party/move/move-compiler-v2/tests/v1.unmatched

@@ -225,17 +222,3 @@ move-compiler/tests/move_check/verification/{
  single_module_invalid.move,
  single_module_valid.move,
 }
-move-compiler/tests/move_check/unit_test/{


After Brian's PR to fix the update tool is landed, these will be recovered.

brmataptos · 2024-04-08T17:13:10Z

third_party/move/move-compiler-v2/tests/visibility-checker/pack_unpack_structs.exp

+error: Invalid pack operation on external type D::G
+   ┌─ tests/visibility-checker/pack_unpack_structs.move:13:16
+   │
+13 │     public fun foo(): C::T {


It would be nice if we could show the module header lines, since that's the controlling factor. Not sure we have those Locs easily available, though.

Please file an issue to follow up on that. It will require adding a field to ModuleData similar to the id_loc field of FunctionData. To initialize that will require adding a parameter id_loc to ModuleBuilder::translate(). Call sites to that should have it available.

brmataptos · 2024-04-08T17:56:45Z

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

+    mid: &ModuleId,
+    pat: &Pattern,
+) {
+    match pat {


You can probably simplify this a bit by using Pattern::visit_pre_post.

vineethk

Looking good so far.

vineethk · 2024-04-09T21:35:13Z

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

+    env.diag_with_labels(Severity::Error, fun_loc, &msg, call_details);
+}
+
+/// check a struct can only be accessed within the module that defines it.


It is a bit misleading to say "struct can only be accessed within the module that defines it" -> struct types are indeed accessible outside of the module, just that certain privileged operations are limited to the defining module (https://aptos.dev/move/book/structs-and-resources#privileged-struct-operations).

Please update docs and maybe function names?

vineethk · 2024-04-09T21:35:59Z

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

 /// 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 structs are not accessed across the module boundary.


same comment as above (certain privileged operations on structs cannot be done across module boundary).

vineethk · 2024-04-09T21:42:33Z

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

+                                id,
+                                "called",
+                                format!(
+                                    "Invalid storage operation on external type `{}`",


Maybe you also fix #12820 with this PR?

Yeah, I think that's the case

vineethk · 2024-04-09T21:47:56Z

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

+                    | Operation::BorrowGlobal(_)
+                    | Operation::MoveFrom
+                    | Operation::MoveTo
+                        if env.get_node_instantiation(*id)[0].get_struct(env).is_some() =>


Instead of doing this check here (the if condition), and then redoing a portion of it again inside, it may be cleaner to move the check inside (and not repeat some of the code).

vineethk · 2024-04-09T21:57:24Z

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

+                                "called",
+                                format!(
+                                    "Invalid storage operation on external type `{}`",
+                                    struct_env.get_full_name_str()


We could perhaps provide more information here to be more heipful to the user, similar to what we do with inaccessible calls (see third_party/move/move-compiler-v2/tests/visibility-checker/v1-typing/module_call_entry_function_was_invalid.exp for an example)?

Maybe we could have something along the lines of: "Invalid operation: storage operation <blah> on type <blah> can only be done within the defining module <blah>" ...

vineethk · 2024-04-09T22:00:25Z

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

+                            id,
+                            "accessed",
+                            format!(
+                                "Invalid access of field `{}` on external type `{}`",


Similar note as above.

Maybe we could have something along the lines of: "Invalid field access: the field blah on type blah can only be accessed withing the defining module blah".

These can be of respite especially in the presence of inlining.

vineethk · 2024-04-09T22:03:34Z

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

+                                id,
+                                "packed",
+                                format!(
+                                    "Invalid pack operation on external type {}",


We can additionally say that only the module defining the struct can create (pack) it.

I find "external type" to provide insufficient guidance to a user (it is external to what? to the module, but that is not clear and can get muddied especially with inlining and where we would point the primary error at).

vineethk · 2024-04-09T23:26:51Z

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

+                    },
+                    _ => {},
+                },
+                ExpData::Assign(_, pat, _) | ExpData::Block(_, pat, _, _) => {


It seems like Lambda also has a pattern, is that a missing case?

This check is performed after inlining. Do we still have Lambda then? cc @brmataptos

Currently, no, but you can throw it in if you want to handle future cases, maybe.

vineethk · 2024-04-09T23:29:03Z

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

-        })
-    }
-} // end 0x42::token
+


Could we add more inline tests, specifically, inline functions that do each of the restricted cases? We may then want to improve error messages for the inlined cases (like is done for visibility of calls).

vineethk · 2024-04-09T23:30:25Z

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

@@ -335,7 +335,7 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
            include: vec!["/acquires-checker/"],
            exclude: vec![],
            exp_suffix: None,
-            options: opts.clone(),
+            options: opts.clone().set_experiment(Experiment::ACCESS_CHECK, false),


Could you add this as a comment please?

vineethk

Very nice! LGTM.

note: I have a comment on a minor typo.

vineethk · 2024-04-10T19:29:26Z

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

+
+/// 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_previleged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {


Suggested change

fn check_previleged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {

fn check_privileged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {

github-actions · 2024-04-10T23:21:49Z

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `7371e5b627d164b3b82d8bc450986701cdf8851e`

Compatibility test results for aptos-node-v1.10.1 ==> 7371e5b627d164b3b82d8bc450986701cdf8851e (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6274 txn/s, latency: 5245 ms, (p50: 5000 ms, p90: 7100 ms, p99: 9300 ms), latency samples: 219600
2. Upgrading first Validator to new version: 7371e5b627d164b3b82d8bc450986701cdf8851e
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1827 txn/s, latency: 15759 ms, (p50: 18600 ms, p90: 22100 ms, p99: 22500 ms), latency samples: 91360
3. Upgrading rest of first batch to new version: 7371e5b627d164b3b82d8bc450986701cdf8851e
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1808 txn/s, latency: 15906 ms, (p50: 19600 ms, p90: 22900 ms, p99: 23200 ms), latency samples: 90420
4. upgrading second batch to new version: 7371e5b627d164b3b82d8bc450986701cdf8851e
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3180 txn/s, latency: 9811 ms, (p50: 10400 ms, p90: 12300 ms, p99: 12600 ms), latency samples: 127200
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 7371e5b627d164b3b82d8bc450986701cdf8851e passed
Test Ok

github-actions · 2024-04-10T23:26:07Z

✅ Forge suite `realistic_env_max_load` success on `7371e5b627d164b3b82d8bc450986701cdf8851e`

two traffics test: inner traffic : committed: 8105 txn/s, latency: 4840 ms, (p50: 4600 ms, p90: 5700 ms, p99: 10800 ms), latency samples: 3501420
two traffics test : committed: 100 txn/s, latency: 1908 ms, (p50: 1900 ms, p90: 2100 ms, p99: 4400 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.204, avg: 0.202", "QsPosToProposal: max: 0.260, avg: 0.235", "ConsensusProposalToOrdered: max: 0.460, avg: 0.416", "ConsensusOrderedToCommit: max: 0.332, avg: 0.313", "ConsensusProposalToCommit: max: 0.745, avg: 0.729"]
Max round gap was 1 [limit 4] at version 1738709. Max no progress secs was 4.650287 [limit 15] at version 1738709.
Test Ok

rahxephon89 commented Apr 8, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-11014 branch 2 times, most recently from 382c47f to eb2c259 Compare April 8, 2024 06:27

rahxephon89 changed the title ~~[WIP][Compiler-v2] check access for structs~~ [Compiler-v2] check access for structs Apr 8, 2024

rahxephon89 marked this pull request as ready for review April 8, 2024 07:04

rahxephon89 requested review from wrwg, brmataptos, fEst1ck and vineethk April 8, 2024 07:04

brmataptos approved these changes Apr 8, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-11014 branch from eb2c259 to 2c35dcd Compare April 8, 2024 22:18

rahxephon89 mentioned this pull request Apr 9, 2024

[Bug][compiler-v2] Internal complier bug "struct not defined" triggered #12820

Closed

vineethk reviewed Apr 9, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-11014 branch from 2c35dcd to 4844c17 Compare April 10, 2024 02:07

rahxephon89 requested a review from vineethk April 10, 2024 02:09

check field

7164b6a

rahxephon89 force-pushed the teng/fix-11014 branch from 4844c17 to e97909a Compare April 10, 2024 16:50

vineethk approved these changes Apr 10, 2024

View reviewed changes

handle comments

541bc7b

rahxephon89 force-pushed the teng/fix-11014 branch from e97909a to 541bc7b Compare April 10, 2024 19:42

rahxephon89 enabled auto-merge (squash) April 10, 2024 19:43

This comment has been minimized.

Sign in to view

Merge branch 'main' into teng/fix-11014

7371e5b

This comment has been minimized.

Sign in to view

rahxephon89 merged commit 59586fe into main Apr 10, 2024
42 of 43 checks passed

rahxephon89 deleted the teng/fix-11014 branch April 10, 2024 23:29

	fn check_previleged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {
	fn check_privileged_operations_on_structs(env: &GlobalEnv, fun_env: &FunctionEnv) {

[Compiler-v2] check access for structs #12821

[Compiler-v2] check access for structs #12821

Conversation

rahxephon89 commented Apr 8, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vineethk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vineethk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Apr 10, 2024

✅ Forge suite compat success on aptos-node-v1.10.1 ==> 7371e5b627d164b3b82d8bc450986701cdf8851e

github-actions bot commented Apr 10, 2024

✅ Forge suite realistic_env_max_load success on 7371e5b627d164b3b82d8bc450986701cdf8851e

rahxephon89 commented Apr 8, 2024 •

edited

Loading

trunk-io bot commented Apr 8, 2024 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `7371e5b627d164b3b82d8bc450986701cdf8851e`

✅ Forge suite `realistic_env_max_load` success on `7371e5b627d164b3b82d8bc450986701cdf8851e`