Skip to content

[vm] check captured value depth when packing closures - #412

Open
fEst1ck wants to merge 1 commit into
movement-network:move-rebase-e33from
fEst1ck:zekun/closure-value-depth-check
Open

[vm] check captured value depth when packing closures#412
fEst1ck wants to merge 1 commit into
movement-network:move-rebase-e33from
fEst1ck:zekun/closure-value-depth-check

Conversation

@fEst1ck

@fEst1ck fEst1ck commented Aug 13, 2026

Copy link
Copy Markdown

Description

Adds a pack-time depth check for closure captured values in the Move VM interpreter.

The #364 backport (upstream aptos-labs#16983) bounds value depth in copy, equals/compare, read_ref, and (de)serialization. However, closures packed from moved captures never go through any of those checks: a loop like let g = f; f = |x| apply(g, x); re-captures by MoveLoc, so the captured chain grows without ever being copied, and PackClosure/PackClosureGeneric construct values of unbounded depth. Function types are the only types whose value depth is not bounded by their static type (Pack for structs is covered by the type-depth check), so this was the remaining gap.

The fix adds a ValueDepthChecker visitor in move-vm-types (exposed as Value::check_max_depth) and calls it on each captured value in both pack arms, gated on enable_depth_checks / max_value_nest_depth.

Boundary semantics: each captured value is counted from its own root (the new closure wrapper does not add a level), so captured values up to depth 128 are allowed and the packed closure may transiently reach total depth 129. This matches both the copy-time boundary (anything legal to copy is legal to capture) and upstream behavior as pinned by the existing function_value_depth.rs e2e test (run2(128) succeeds, run2(129) aborts). A closure packed at the limit aborts on later copy/compare/store, which count from the closure root; this transient overshoot is inherent (struct-packing over a max-depth closure field creates it too) and is documented and pinned by a test.

How Has This Been Tested?

  • New transactional test move-vm/transactional-tests/tests/instructions/closure_pack_depth.move pins the boundary with move-only captures (isolating the pack-time check): chain(128) packs and runs; chain(129) aborts with VM_MAX_VALUE_DEPTH_REACHED at the PackClosure; chain(128) followed by an explicit copy aborts at the CopyLoc.
  • Existing e2e regression test function_value_depth::test_vm_value_too_deep_with_function_values (from [vm] backport VM value depth checks (#16983) #364) passes: run2(128) succeeds, run2(129) aborts.
  • Full move-vm transactional-tests suite: 680/680 pass across all configs.
  • move-vm-types value_depth_tests unit tests: all pass.

Key Areas to Review

  • Boundary choice (128/129 vs 127/128): the check deliberately excludes the closure wrapper so pack agrees with the copy check and with upstream; the alternative (counting the wrapper) would reject values the VM just allowed to be copied and would break the [vm] backport VM value depth checks (#16983) #364 e2e expectations. See the doc comment on check_captured_value_depths in interpreter.rs.
  • ValueDepthChecker level counting (values_impl.rs): struct/vector/closure children at +1; primitive-vector contents add no level and are checked O(1) via visit_vec_* overrides, matching copy_value and the serializer; references descend (unreachable for captures, which cannot contain references).
  • Check placement: after popn of the captures and before LazyLoadedFunction::new_resolved, preserving the fix: Security nits for MoveVM #354 verify-before-pack order.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

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

Add a ValueDepthChecker visitor and check each captured value against
max_value_nest_depth in PackClosure/PackClosureGeneric, closing the
move-capture path that bypassed the aptos-labs#364 copy-time checks. Captured
values count from their own root (boundary 128/129, matching upstream);
a transactional test pins the pack and copy boundaries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fEst1ck
fEst1ck marked this pull request as ready for review August 13, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant