[mono-move] Migrate from using the old type representation to the new one#19530
[mono-move] Migrate from using the old type representation to the new one#19530
Conversation
vineethk
changed the title
vineethk
force-pushed
the
vk/migrate-type-representation
branch
from
8ea5c74 to
b19ae09
Compare
vineethk
requested review from
calintat,
georgemitenkov and
vgao1996
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Differential Security Review — PR #19530: Migrate to new type representation
Overall risk: Low. This is a compiler-internal refactor (mono-move specializer/orchestrator), well-isolated from on-chain execution paths. The main findings are a latent frame-layout correctness bug and a panic on valid generic Move bytecode, both in the experimental compiler pipeline.
Scope
Codebase size: MEDIUM (62 changed files, ~10 Rust source files with substantive logic changes). Strategy: FOCUSED — full analysis on high-risk changed files, surface scan on mechanical changes.
Changed subsystems: mono-move/core, mono-move/global-context, mono-move/specializer, mono-move/orchestrator. All experimental compiler-pipeline code, not deployed on-chain execution paths (aptos-vm, move-vm, consensus, storage, mempool — none touched).
Summary of findings: 2 findings (1 MEDIUM, 1 LOW), plus 2 notes in test coverage / recommendations.
Findings
[MEDIUM] args_size excludes inter-parameter alignment padding
File: third_party/move/mono-move/specializer/src/pipeline.rs:37–40
Blast radius: 1 call site (build_executable via destack_and_lower_module)
Test coverage: Untested for mixed-alignment parameters
Description: args_size is computed as the sum of individual parameter slot sizes:
let args_size = ctx.home_slots[..func_ir.num_params as usize]
.iter()
.map(|s| s.size as usize)
.sum::<usize>();layout_slots (introduced in this PR, lower/context.rs:130–145) now inserts alignment padding between slots via align_up(offset, alignment). For a parameter list that includes type pairs of differing alignment — e.g., a bool (1-byte) followed by a u128 (16-byte) — the second parameter starts at an offset that is not sum(previous sizes), so args_size < actual_end_of_args.
The runtime uses args_size as the start of the zero-initialization range on frame entry (when zero_frame is true):
// interpreter.rs:682
let zero_size = callee.extended_frame_size - callee.args_size;
std::ptr::write_bytes(new_fp.add(callee.args_size), 0, zero_size);With args_size < actual_end_of_args, write_bytes zeroes bytes inside the argument area that the caller already wrote, corrupting the tail portion of the argument region.
Historical context: This padding was not present in the pre-PR lower/context.rs, which accumulated size linearly with no alignment gaps. The introduction of layout_slots created the inconsistency.
Concrete impact: zero_frame is currently always set to false in the orchestrator (builder.rs:150), so this path is not executed today and no argument corruption occurs in the current code. The bug becomes active as soon as zero_frame: true is wired in for GC-safe functions (which is the documented intent). The correct formula is home_slots[num_params-1].offset + home_slots[num_params-1].size for a non-empty param list, or 0 for a zero-param function.
[LOW] todo! panic on modules with generic struct definitions
File: third_party/move/mono-move/orchestrator/src/builder.rs:202
Blast radius: All callers of build_executable / resolve_types
Test coverage: Not exercised by the test suite (no generic-struct test cases)
Description: resolve_struct_def panics unconditionally when the struct handle has type parameters:
if !handle.type_parameters.is_empty() {
todo!("Generic structs / enums not yet supported");
}resolve_types iterates all struct defs unconditionally; there is no pre-filter. Any compiled Move module containing a generic struct (e.g., struct Pair<T, U>) triggers this panic during type resolution. The Move standard library contains many generic structs; a pipeline that compiles standard-library-dependent modules will panic.
Concrete impact: This is a compiler-pipeline crash on valid Move bytecode, not an on-chain or consensus path. It is a development-completeness issue rather than a security issue. Still worth tracking explicitly as it will surface when modules with generics are added to the test suite.
Test Coverage Analysis
| Changed Area | Coverage | Note |
|---|---|---|
layout_slots with mixed-alignment types |
Untested | All test cases use u64/primitives with uniform 8-byte alignment |
intern_struct_type with pre-existing entry (dedup path) |
Untested | Double-allocation is harmless but the dedup path has no test |
walk_sig_token / TypeInternerKey for TypeParam |
Partially tested | Indirectly tested via type-param interning; no direct round-trip test |
Generic struct resolution (resolve_struct_def) |
Untested | todo! path never reached |
Coverage assessment: The new layout_slots alignment logic is only exercised with same-size types; the correctness of args_size under mixed alignment has no coverage.
Blast Radius
| Changed Function | Non-Test Callers | Classification |
|---|---|---|
translate_module |
1 (destack) |
LOW |
SsaConverter::struct_type / struct_inst_type |
~20 call sites within ssa_conversion.rs |
MEDIUM (all internal) |
intern_struct_type / intern_enum_type |
1 (resolve_struct_def) |
LOW |
layout_slots |
2 (try_build_context_inner) |
LOW |
All blast radius is contained within the mono-move compiler pipeline; no callers in aptos-vm, move-vm-runtime, or any on-chain execution path.
Notes
-
&'staticlifetime laundering (core/src/types.rs:109–125):view_type(),view_type_list(),view_name()return&'staticreferences from arena memory. This is intentional (lifetime cannot be expressed in Rust) and well-documented, but it is convention-enforced rather than type-enforced. A stored reference surviving an arena reset would be undefined behavior. The comment is correct; the API should be used only within the execution-phase scope. -
Recursive type interning (
context/types.rsTypeInternerKey::hash,sig_walk.rswalk_sig_token): Both are marked// TODO: non-recursive implementation. Recursion depth is bounded at ~256 for bytecode-deserialized tokens (enforced bySIGNATURE_TOKEN_DEPTH_MAXin the binary-format deserializer). No stack overflow risk for standard Move modules. -
SignatureTokenKey/TypeInternerKeyhash consistency: The two key types hash struct/enum identity using structural string content (address, module name, struct name), not pointer values. Correctness depends onExecutableIdbeing constructed with the same string values asstruct_info_at's module metadata lookup. This invariant holds for the current builder path but is not type-enforced.
Recommendations
Before enabling zero_frame: true
- Fix
args_sizeinpipeline.rs:37–40: replacesum of sizeswithlast_param_slot.offset + last_param_slot.size(handle the zero-param case with0).
Before adding generic-struct support
- Replace
todo!("Generic structs / enums not yet supported")inbuilder.rs:202with a properbail!that returnsErrrather than panicking, so callers can handle the unsupported case gracefully.
Test coverage
- Add a test case with mixed-alignment parameters (e.g.,
bool+u128) to cover thelayout_slotsalignment behavior andargs_sizecomputation. - Add a test case for a module with a generic struct to exercise the
todo!path explicitly (confirming it errors cleanly once thetodo!is converted tobail!).
Sent by Cursor Automation: Security Review Bot
vineethk
force-pushed
the
vk/eliminate-types-in-op-stack
branch
from
3a2d33e to
796284c
Compare
vineethk
force-pushed
the
vk/migrate-type-representation
branch
from
b19ae09 to
4542ba1
Compare
vineethk
force-pushed
the
vk/eliminate-types-in-op-stack
branch
from
796284c to
6223347
Compare
vineethk
force-pushed
the
vk/migrate-type-representation
branch
from
4542ba1 to
c689658
Compare
vineethk
force-pushed
the
vk/migrate-type-representation
branch
from
c689658 to
2bfb8db
Compare
|
@georgemitenkov addressed comments, PTAL |
calintat
left a comment
calintat
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments (feel free to ignore).
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.
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.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
3 participants
Description
Until now, the specializer operated on a placeholder (old) type representation. The real, canonical runtime type model lived in mono-move-global-context. Lowered IR carried raw file-format indices (StructDefinitionIndex, StructDefInstantiationIndex, etc.) that would need to be re-resolved at runtime against the loader's real type graph.
This PR closes that gap. The specializer now produces IR that references the same interned Type pointers the runtime uses, end-to-end.
Some key changes:
Typemoves fromglobal-contextintomono-move-core. Core now owns the runtime type model (types.rs,InternedType,InternedTypeList) so both thespecializerandglobal-contextcan depend on it without thespecializerhaving to depend onglobal-contextinternals or onmove-vm-types.stackless_exec_ir::InstrcarriesInternedTypeinstead of file-format indices. Pack/Unpack/PackVariant/Exists/MoveFrom/... now hold resolved interned struct and enum types (plus anInternedTypeListfor generic instantiations) rather than per-moduleStructDefinitionIndexhandles. Pointer equality of these handles is structural equality.walk_sig_tokenhelper inglobal-contextvia a smallStructResolvertrait, sospecializerandglobal-contextshare one signature-token walker instead of maintaining two copies.mono-move-orchestratorcrate. Pipeline orchestration (resolve types → run specializer → assemble Executable) is split out of specializer. This keeps the specializer a pure IR transform and gives us a clean seam for future passes (monomorphization, GC safe-point layout, etc.).global-contextno longer depends onspecializerormove-vm-types;specializerno longer depends onmove-vm-typesormove-asm.Out of scope:
How Has This Been Tested?
Existing masm/ and move/ golden test suites cover the pipeline end-to-end; .exp changes in this PR are driven by the switch to interned-type.
The test layout was reorganized to follow the new crate boundaries — tests live with the crate whose seam they actually exercise, not with the crate whose internals used to drive the pipeline.
End-to-end .masm / .move test corpus moved specializer/ → orchestrator/. The golden-file suite (the masm_runner / move_runner datatest harness in tests/testsuite.rs plus all tests/test_cases/masm/.{masm,exp} and tests/test_cases/move/.{move,exp}) now lives under orchestrator/tests/. That crate is the one that actually drives the full pipeline (resolve types → specialize → assemble Executable), so the E2E baselines belong there. The specializer crate, which is now a pure IR transform, no longer has its own test binary.
Type-interner unit tests moved global-context/ → orchestrator/. types_tests.rs moved alongside the E2E suite. This is a pragmatic consequence of the Type relocation: the interner is still implemented in global-context, but exercising it end-to-end — interning types produced by resolving real compiled modules — now requires the orchestrator to stitch core, global-context, and specializer together. Pure in-memory interner tests that don't need a module could move back to global-context later.
Gas integration test moved global-context/ → testsuite/. gas_test.rs relocated to testsuite/tests/, matching the broader pattern of cross-crate integration tests living in the shared testsuite crate.
Note
Medium Risk
Touches core IR/type representations and refactors the module-loading/specialization pipeline across multiple crates, so regressions could surface as incorrect type/layout resolution or lowering failures. Changes are internal but wide-reaching and involve unsafe arena pointer usage and new interning APIs.
Overview
Moves the canonical runtime type model into
mono-move-core(types.rs) and updates executables to storeInternedType/InternedTypeListplus struct/enum metadata, enabling pointer-stable type identity end-to-end.Refactors
mono-move-global-contextto re-export core types, add public type-interning helpers (e.g.intern_*,type_data), and share signature-token walking via a newwalk_sig_token+StructResolverseam; executable caching now consumesmono-move-core::Executable.Introduces a new
mono-move-orchestratorcrate that resolves module types, runs the specializer, patches call sites, and assemblesExecutables; the previous executable builder is removed fromglobal-context, themseir-compilerCLI and golden tests are moved under orchestrator, andspecializersheds CLI/tests and extra deps while updating passes (e.g. slot allocation) to carry interned types.Reviewed by Cursor Bugbot for commit c7d6184. Bugbot is set up for automated code reviews on this repo. Configure here.