Add actor call stack to call manager + detect reentrant upgrades in syscall

Author: fridrik01

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<M: Machine> {
     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<ActorID, i32>,
+    /// 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<ActorID, i32> {
+    fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> {
         &self.actor_call_stack
     }
 
-    fn get_actor_call_stack_mut(&mut self) -> &mut HashMap<ActorID, i32> {
-        &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::<K>(from, to, entrypoint, params, value, read_only)
+        self.actor_call_stack_push(to, &entrypoint);
+        let res = self.send_resolved::<K>(from, to, entrypoint, params, value, read_only);
+        self.actor_call_stack_pop();
+
+        res
     }
 
     /// Send with resolved addresses.

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,17 @@ pub trait CallManager: 'static {
         delegated_address: Option<Address>,
     ) -> Result<()>;
 
-    fn get_actor_call_stack(&self) -> &HashMap<ActorID, i32>;
-    fn get_actor_call_stack_mut(&mut self) -> &mut HashMap<ActorID, i32>;
+    /// Based on current actor stack, checks if the actor with the given ID can be upgraded.
+    //fn can_actor_upgrade(&self, actor_id: ActorID) -> bool;
+
+    // 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<Option<ActorID>>;
@@ -210,6 +218,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 +240,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,
         }
     }
 

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::<K>(
@@ -879,12 +876,26 @@ 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)
+        while let Some(tuple) = iter.next() {
+            if tuple.0 != self.actor_id {
+                return false;
+            } else if tuple.1 != UPGRADE_FUNC_NAME {
+                return false;
+            }
+        }
+
+        true
     }
 
     fn upgrade_actor<K: Kernel>(

fvm/src/kernel/mod.rs

@@ -215,7 +215,7 @@ pub trait ActorOps {
         params_id: BlockId,
     ) -> Result<SendResult>;
 
-    fn is_actor_on_call_stack(&self) -> bool;
+    fn can_actor_upgrade(&self) -> bool;
 
     /// Installs actor code pointed by cid
     #[cfg(feature = "m2-native")]

fvm/src/syscalls/actor.rs

@@ -121,11 +121,10 @@ pub fn upgrade_actor<K: Kernel>(
         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"
         ));
     };
 

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<ActorID, i32> {
+    fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> {
         todo!()
     }
 
-    fn get_actor_call_stack_mut(&mut self) -> &mut HashMap<ActorID, i32> {
+    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!()
     }
 

testing/conformance/src/vm.rs

@@ -303,8 +303,8 @@ where
         self.0.upgrade_actor::<Self>(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()
     }
 }
 

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(),