Skip to content

[vm] Fail closed on aliased RefCell borrows in Move values - #426

Open
sausagee wants to merge 1 commit into
m1from
cursor/vm-refcell-alias-fail-closed-4e1c
Open

[vm] Fail closed on aliased RefCell borrows in Move values#426
sausagee wants to merge 1 commit into
m1from
cursor/vm-refcell-alias-fail-closed-4e1c

Conversation

@sausagee

@sausagee sausagee commented Sep 6, 2026

Copy link
Copy Markdown

Description

Clean-room rewrite (not a cherry-pick or copy of aptos-labs sources). Intent only: aliased paired RefCell::borrow_mut must not abort the interpreter thread (aptos-labs#20439).

Container::swap_contents and VectorRef::move_range took two exclusive borrow_muts. Move's borrow checker promises the cells are distinct; if that invariant is broken, stacked borrow_mut panicked the VM thread.

This change acquires the pair through a new helper, exclusive_cell_pair:

  1. Reject Rc::ptr_eq immediately.
  2. Otherwise try_borrow_mut each cell.
  3. On either failure, return UNKNOWN_INVARIANT_VIOLATION_ERROR.

Verified distinct cells still swap / move as before. No other crate in the monorepo is touched.

Invariant: a broken aliasing (or already-borrowed) pair on these paths surfaces as a catchable VM status, never a Rust panic.

Follows SECURITY.md / RUST_SECURE_CODING.md: Result instead of panic, documented safety invariant, no unsafe.

How Has This Been Tested?

  • cargo test -p move-vm-types70 passed, 0 failed
  • container_self_swap_returns_status_instead_of_panicking — specialized vector, generic struct, generic vector; self-swap is UNKNOWN_INVARIANT_VIOLATION_ERROR and leaves contents unchanged
  • move_range_on_aliased_vector_refs_returns_status_instead_of_panicking — distinct vectors still transfer; aliased refs return the same status
  • Existing test_mem_swap covers the happy path for distinct container refs

Key Areas to Review

  • exclusive_cell_pair in third_party/move/move-vm/types/src/values/values_impl.rs — fail-closed helper used by both hardened paths
  • Confirm this is an independent rewrite (helper + ptr_eq first, not a port of the upstream patch)

Type of Change

  • Bug fix

Which Components or Systems Does This Change Impact?

  • Move/Aptos Virtual Machine

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
Open in Web Open in Cursor 

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Container::swap_contents and VectorRef::move_range took paired
borrow_mut on cells the verifier promises are distinct. A broken
alias panicked the interpreter thread.

Acquire the pair through exclusive_cell_pair (ptr_eq then
try_borrow_mut) and return UNKNOWN_INVARIANT_VIOLATION_ERROR.
Independent rewrite; no aptos-labs sources were copied.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
@sausagee
sausagee marked this pull request as ready for review September 6, 2026 05:05
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.

2 participants