diff --git a/third_party/move/move-vm/types/src/values/value_tests.rs b/third_party/move/move-vm/types/src/values/value_tests.rs index 1563c94df05..c4ea32f874a 100644 --- a/third_party/move/move-vm/types/src/values/value_tests.rs +++ b/third_party/move/move-vm/types/src/values/value_tests.rs @@ -5,7 +5,7 @@ use crate::{loaded_data::runtime_types::TypeBuilder, values::*, views::*}; use claims::{assert_err, assert_ok}; use move_binary_format::errors::*; -use move_core_types::{account_address::AccountAddress, u256::U256}; +use move_core_types::{account_address::AccountAddress, u256::U256, vm_status::StatusCode}; #[test] fn locals() -> PartialVMResult<()> { @@ -310,6 +310,68 @@ fn test_mem_swap() -> PartialVMResult<()> { Ok(()) } +fn assert_invariant_violation(err: PartialVMError) { + assert_eq!( + err.major_status(), + StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR + ); +} + +/// Two `Reference`s into the same container must not panic the VM thread. +/// `swap_contents` fails closed with an invariant-violation status. +#[test] +fn container_self_swap_returns_status_instead_of_panicking() -> PartialVMResult<()> { + let mut locals = Locals::new(3); + locals.store_loc(0, Value::vector_u64(vec![1, 2, 3]), false)?; + locals.store_loc(1, Value::struct_(Struct::pack(vec![Value::u16(9)])), false)?; + locals.store_loc( + 2, + Value::vector_for_testing_only(vec![Value::u64(7), Value::u64(8)]), + false, + )?; + + let borrow = + |ls: &Locals, idx: usize| ls.borrow_loc(idx).unwrap().value_as::().unwrap(); + + for idx in 0..3 { + let before = borrow(&locals, idx).read_ref()?; + let err = borrow(&locals, idx) + .swap_values(borrow(&locals, idx)) + .unwrap_err(); + assert_invariant_violation(err); + assert!(borrow(&locals, idx).read_ref()?.equals(&before)?); + } + + Ok(()) +} + +/// `move_range` on two refs to the same vector must not panic. Distinct +/// vectors still transfer the range. +#[test] +fn move_range_on_aliased_vector_refs_returns_status_instead_of_panicking() -> PartialVMResult<()> { + let mut locals = Locals::new(2); + locals.store_loc(0, Value::vector_u64(vec![10, 20, 30]), false)?; + locals.store_loc(1, Value::vector_u64(vec![40, 50]), false)?; + + let ty = TypeBuilder::with_limits(10, 10).create_u64_ty(); + let as_vector_ref = + |ls: &Locals, idx: usize| ls.borrow_loc(idx).unwrap().value_as::().unwrap(); + + let src = as_vector_ref(&locals, 0); + let dst = as_vector_ref(&locals, 1); + assert_ok!(VectorRef::move_range(&src, 0, 1, &dst, 0, &ty)); + assert_eq!(src.length_as_usize(&ty)?, 2); + assert_eq!(dst.length_as_usize(&ty)?, 3); + + let same_a = as_vector_ref(&locals, 0); + let same_b = as_vector_ref(&locals, 0); + let err = VectorRef::move_range(&same_a, 0, 1, &same_b, 0, &ty).unwrap_err(); + assert_invariant_violation(err); + assert_eq!(same_a.length_as_usize(&ty)?, 2); + + Ok(()) +} + #[cfg(test)] mod native_values { use super::*; diff --git a/third_party/move/move-vm/types/src/values/values_impl.rs b/third_party/move/move-vm/types/src/values/values_impl.rs index c1ecf88ceaf..46047518bce 100644 --- a/third_party/move/move-vm/types/src/values/values_impl.rs +++ b/third_party/move/move-vm/types/src/values/values_impl.rs @@ -33,7 +33,7 @@ use serde::{ Deserialize, }; use std::{ - cell::RefCell, + cell::{RefCell, RefMut}, cmp::Ordering, fmt::{self, Debug, Display, Formatter}, iter, mem, @@ -336,6 +336,33 @@ fn take_unique_ownership(r: Rc>) -> PartialVMResult { } } +/// Fail-closed exclusive access to two `RefCell`s that the bytecode verifier +/// promises are distinct. +/// +/// Invariant: a broken aliasing (or already-borrowed) pair must surface as +/// `UNKNOWN_INVARIANT_VIOLATION_ERROR`. Never call stacked `borrow_mut` on +/// the pair — that panics the interpreter thread instead of producing a +/// catchable VM status. +fn exclusive_cell_pair<'a, T>( + left: &'a Rc>, + right: &'a Rc>, +) -> PartialVMResult<(RefMut<'a, T>, RefMut<'a, T>)> { + if Rc::ptr_eq(left, right) { + return Err(overlapping_mut_cells()); + } + let left_mut = left.try_borrow_mut().map_err(|_| overlapping_mut_cells())?; + let right_mut = right + .try_borrow_mut() + .map_err(|_| overlapping_mut_cells())?; + Ok((left_mut, right_mut)) +} + +fn overlapping_mut_cells() -> PartialVMError { + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message( + "cannot take exclusive RefCell access on overlapping or already-borrowed cells".to_string(), + ) +} + impl ContainerRef { fn container(&self) -> &Container { match self { @@ -1294,44 +1321,45 @@ impl_vm_value_from_primitive!(AccountAddress, Address); * *************************************************************************************/ impl Container { - /// Swaps contents of two mutable references. + /// Exchange the inner buffers of two containers. /// - /// Precondition for this funciton is that `self` and `other` are required to be - /// distinct references. - /// Move will guarantee that invariant, because it prevents from having two - /// mutable references to the same value. + /// Move's reference safety requires `self` and `other` to name distinct + /// cells. If that invariant is broken, this method returns an invariant- + /// violation status instead of panicking on a stacked `borrow_mut`. fn swap_contents(&self, other: &Self) -> PartialVMResult<()> { use Container::*; + fn swap_cells(left: &Rc>, right: &Rc>) -> PartialVMResult<()> { + let (mut a, mut b) = exclusive_cell_pair(left, right)?; + mem::swap(&mut *a, &mut *b); + Ok(()) + } + match (self, other) { - (Vec(l), Vec(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (Struct(l), Struct(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), + (Vec(l), Vec(r)) => swap_cells(l, r), + (Struct(l), Struct(r)) => swap_cells(l, r), - (VecBool(l), VecBool(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecAddress(l), VecAddress(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), + (VecBool(l), VecBool(r)) => swap_cells(l, r), + (VecAddress(l), VecAddress(r)) => swap_cells(l, r), - (VecU8(l), VecU8(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecU16(l), VecU16(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecU32(l), VecU32(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecU64(l), VecU64(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecU128(l), VecU128(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), - (VecU256(l), VecU256(r)) => mem::swap(&mut *l.borrow_mut(), &mut *r.borrow_mut()), + (VecU8(l), VecU8(r)) => swap_cells(l, r), + (VecU16(l), VecU16(r)) => swap_cells(l, r), + (VecU32(l), VecU32(r)) => swap_cells(l, r), + (VecU64(l), VecU64(r)) => swap_cells(l, r), + (VecU128(l), VecU128(r)) => swap_cells(l, r), + (VecU256(l), VecU256(r)) => swap_cells(l, r), ( Locals(_) | Vec(_) | Struct(_) | VecBool(_) | VecAddress(_) | VecU8(_) | VecU16(_) | VecU32(_) | VecU64(_) | VecU128(_) | VecU256(_), _, - ) => { - return Err( - PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR).with_message(format!( - "cannot swap container values: {:?}, {:?}", - self, other - )), - ) - }, + ) => Err( + PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR).with_message(format!( + "cannot swap container values: {:?}, {:?}", + self, other + )), + ), } - - Ok(()) } } @@ -2891,8 +2919,9 @@ impl VectorRef { /// In the `to` vector, elements after the `insert_position` are moved to the right to make space for new elements /// (i.e. range is inserted, while the order of the rest of the elements is kept). /// - /// Precondition for this function is that `from` and `to` vectors are required to be distinct - /// Move will guaranteee that invariant, because it prevents from having two mutable references to the same value. + /// `from` and `to` must be distinct cells. The verifier enforces that; if + /// the pair aliases we fail closed with an invariant-violation status + /// rather than panicking on stacked `borrow_mut`. pub fn move_range( from_self: &Self, removal_position: usize, @@ -2913,8 +2942,7 @@ impl VectorRef { macro_rules! move_range { ($from:expr, $to:expr) => {{ - let mut from_v = $from.borrow_mut(); - let mut to_v = $to.borrow_mut(); + let (mut from_v, mut to_v) = exclusive_cell_pair($from, $to)?; if removal_position.checked_add(length).map_or(true, |end| end > from_v.len()) || insert_position > to_v.len() {