From 0f65803a27e02139390aef38776c76017c65afb0 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 19 Oct 2023 18:35:45 +0000 Subject: [PATCH] Add actor call stack to call manager + detect reentrant upgrades in syscall --- fvm/src/call_manager/default.rs | 23 +++++++++++++------- fvm/src/call_manager/mod.rs | 18 +++++++++++----- fvm/src/kernel/default.rs | 35 +++++++++++++++++++------------ fvm/src/kernel/mod.rs | 2 +- fvm/src/syscalls/actor.rs | 5 ++--- fvm/tests/dummy.rs | 9 +++++--- testing/conformance/src/vm.rs | 4 ++-- testing/integration/tests/main.rs | 2 +- 8 files changed, 62 insertions(+), 36 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index d2f46e5f6..4cd6abcc6 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -14,7 +14,6 @@ use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; use fvm_shared::{ActorID, METHOD_SEND}; use num_traits::Zero; -use std::collections::HashMap; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; use super::{Backtrace, CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; @@ -76,8 +75,8 @@ pub struct InnerDefaultCallManager { limits: M::Limiter, /// Accumulator for events emitted in this call stack. events: EventsAccumulator, - /// A map of ActorID and how often they appear on the call stack. - actor_call_stack: HashMap, + /// The actor call stack (ActorID and entrypoint name tuple). + actor_call_stack: Vec<(ActorID, &'static str)>, } #[doc(hidden)] @@ -162,7 +161,7 @@ where limits, events: Default::default(), state_access_tracker, - actor_call_stack: HashMap::new(), + actor_call_stack: vec![], }))) } @@ -331,12 +330,16 @@ where self.nonce } - fn get_actor_call_stack(&self) -> &HashMap { + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { &self.actor_call_stack } - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { - &mut self.actor_call_stack + fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint) { + self.actor_call_stack + .push((actor_id, entrypoint.func_name())) + } + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { + self.actor_call_stack.pop() } fn next_actor_address(&self) -> Address { @@ -639,7 +642,11 @@ where }, }; - self.send_resolved::(from, to, entrypoint, params, value, read_only) + self.actor_call_stack_push(to, &entrypoint); + let res = self.send_resolved::(from, to, entrypoint, params, value, read_only); + self.actor_call_stack_pop(); + + res } /// Send with resolved addresses. diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 4ef7fd3ee..718a351a1 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -7,7 +7,6 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum}; -use std::collections::HashMap; use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; @@ -120,8 +119,14 @@ pub trait CallManager: 'static { delegated_address: Option
, ) -> Result<()>; - fn get_actor_call_stack(&self) -> &HashMap; - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap; + // returns the actor call stack + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)>; + + /// add an actor to the calling stack. + fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint); + + /// pop an actor from the calling stack. + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)>; /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; @@ -210,6 +215,9 @@ pub enum Entrypoint { Upgrade(UpgradeInfo), } +pub static INVOKE_FUNC_NAME: &str = "invoke"; +pub static UPGRADE_FUNC_NAME: &str = "upgrade"; + impl std::fmt::Display for Entrypoint { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -229,8 +237,8 @@ impl Entrypoint { fn func_name(&self) -> &'static str { match self { - Entrypoint::Invoke(_) => "invoke", - Entrypoint::Upgrade(_) => "upgrade", + Entrypoint::Invoke(_) => INVOKE_FUNC_NAME, + Entrypoint::Upgrade(_) => UPGRADE_FUNC_NAME, } } diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 9ed60f14e..3a51a2d19 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -30,7 +30,10 @@ use super::blocks::{Block, BlockRegistry}; use super::error::Result; use super::hash::SupportedHashes; use super::*; -use crate::call_manager::{CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; +use crate::call_manager::{ + CallManager, Entrypoint, InvocationResult, INVOKE_FUNC_NAME, NO_DATA_BLOCK_ID, + UPGRADE_FUNC_NAME, +}; use crate::externs::{Chain, Consensus, Rand}; use crate::gas::GasTimer; use crate::init_actor::INIT_ACTOR_ID; @@ -137,12 +140,6 @@ where return Err(syscall_error!(LimitExceeded; "cannot store return block").into()); } - self.call_manager - .get_actor_call_stack_mut() - .entry(self.actor_id) - .and_modify(|count| *count += 1) - .or_insert(1); - // Send. let result = self.call_manager.with_transaction(|cm| { cm.call_actor::( @@ -879,12 +876,24 @@ where .create_actor(code_id, actor_id, delegated_address) } - fn is_actor_on_call_stack(&self) -> bool { - self.call_manager - .get_actor_call_stack() - .get(&self.actor_id) - .map(|count| *count > 0) - .unwrap_or(false) + fn can_actor_upgrade(&self) -> bool { + let mut iter = self.call_manager.get_actor_call_stack().iter(); + + // find the first position of this actor on the call stack + let position = iter.position(|&tuple| tuple == (self.actor_id, INVOKE_FUNC_NAME)); + if position.is_none() { + return true; + } + + // make sure that no other actor appears on the call stack after 'position' (unless its + // a recursive upgrade call which is allowed) + for tuple in iter { + if tuple.0 != self.actor_id || tuple.1 != UPGRADE_FUNC_NAME { + return false; + } + } + + true } fn upgrade_actor( diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 9af6c7851..16ce621cd 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -215,7 +215,7 @@ pub trait ActorOps { params_id: BlockId, ) -> Result; - fn is_actor_on_call_stack(&self) -> bool; + fn can_actor_upgrade(&self) -> bool; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 2f694b1b6..6b5c9eaa5 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -121,11 +121,10 @@ pub fn upgrade_actor( Err(err) => return err.into(), }; - // actor cannot be upgraded if its already on the call stack - if context.kernel.is_actor_on_call_stack() { + if !context.kernel.can_actor_upgrade() { return ControlFlow::Error(syscall_error!( Forbidden; - "cannot upgrade actor if it is already on the call stack" + "calling upgrade on actor already on call stack is forbidden" )); }; diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 1994a79e4..424cdedda 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0, MIT use std::borrow::Borrow; use std::cell::RefCell; -use std::collections::HashMap; use std::rc::Rc; use anyhow::Context; @@ -359,11 +358,15 @@ impl CallManager for DummyCallManager { todo!() } - fn get_actor_call_stack(&self) -> &HashMap { + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { todo!() } - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { + fn actor_call_stack_push(&mut self, _actor_id: ActorID, _entrypoint: &Entrypoint) { + todo!() + } + + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { todo!() } diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index b43421da7..c8b4d49ef 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -303,8 +303,8 @@ where self.0.upgrade_actor::(new_code_cid, params_id) } - fn is_actor_on_call_stack(&self) -> bool { - self.0.is_actor_on_call_stack() + fn can_actor_upgrade(&self) -> bool { + self.0.can_actor_upgrade() } } diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index fc3de3e9c..d621c7352 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -1189,7 +1189,7 @@ fn upgrade_actor_test() { .unwrap(); let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 444); + assert_eq!(val, 444, "{:?}", res.failure_info); assert!( res.msg_receipt.exit_code.is_success(),