From 0d457d3d52fe2dd7dc6f48c54fb7294f0459e78a Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 2 Aug 2023 13:23:56 -0700 Subject: [PATCH] fix(fvm): fix (and prevent) two bugs in out of gas handling. My last commit introduced a bug where I ignored the error from a call to `CallManager::charge_gas` when we _meant_ to simply ignore the gas timer, not the entire error. This bug was in the charge for `on_method_return`. So I decided to remove the `must_use` marker from gas timers _in general_ so we didn't have to explicitly ignore the results. We generally don't care about these timers anyways. While doing that, I found another bug in the `tipset_cid` syscall where we made the same mistake. this issue is tricker because this code is already deployed to mainnet, but we should be able to fix it without any consensus issues. Basically, right now: 1. We won't detect the out of gas condition when we should. 2. We'll fetch the requested tipset CID from state. 3. We'll return it to the user. 4. The actor (the EVM) will try to charge a bit of gas when handling the return value from the syscall. 5. The actor will run out of gas and abort with an OutOfGas. Now, we'll just abort with an OutOfGas error immediately. --- fvm/src/call_manager/default.rs | 28 ++++++++++++---------------- fvm/src/gas/mod.rs | 4 ++-- fvm/src/gas/timer.rs | 1 - fvm/src/kernel/default.rs | 8 +++----- fvm/src/syscalls/bind.rs | 2 +- fvm/src/syscalls/mod.rs | 5 ++--- 6 files changed, 20 insertions(+), 28 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index ba599addd4..d3ac283323 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -398,7 +398,7 @@ where None => { // We charge for creating the actor (storage) but not for address assignment as the // init actor has already handled that for us. - let _ = self.charge_gas(self.price_list().on_create_actor(false))?; + self.charge_gas(self.price_list().on_create_actor(false))?; ActorState::new_empty(code_id, delegated_address) } }; @@ -422,8 +422,7 @@ where return Ok(Some(id)); } if !self.state_access_tracker.get_address_lookup_state(address) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_resolve_address())?; } let id = self.state_tree().lookup_id(address)?; @@ -436,8 +435,7 @@ where fn get_actor(&self, id: ActorID) -> Result> { let access = self.state_access_tracker.get_actor_access_state(id); if access < Some(ActorAccessState::Read) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_actor_lookup())?; } let actor = self.state_tree().get_actor(id)?; @@ -448,13 +446,11 @@ where fn set_actor(&mut self, id: ActorID, state: ActorState) -> Result<()> { let access = self.state_access_tracker.get_actor_access_state(id); if access < Some(ActorAccessState::Read) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_actor_lookup())?; } if access < Some(ActorAccessState::Updated) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_actor_update())?; } self.state_tree_mut().set_actor(id, state); @@ -465,13 +461,11 @@ where fn delete_actor(&mut self, id: ActorID) -> Result<()> { let access = self.state_access_tracker.get_actor_access_state(id); if access < Some(ActorAccessState::Read) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_actor_lookup())?; } if access < Some(ActorAccessState::Updated) { - let _ = self - .gas_tracker + self.gas_tracker .apply_charge(self.price_list().on_actor_update())?; } self.state_tree_mut().delete_actor(id); @@ -536,7 +530,7 @@ where fn create_actor_from_send(&mut self, addr: &Address, act: ActorState) -> Result { // This will charge for the address assignment and the actor storage, but not the actor // lookup/update (charged below in `set_actor`). - let _ = self.charge_gas(self.price_list().on_create_actor(true))?; + self.charge_gas(self.price_list().on_create_actor(true))?; let addr_id = self.state_tree_mut().register_new_address(addr)?; self.state_access_tracker.record_lookup_address(addr); @@ -778,7 +772,7 @@ where }); // Process the result, updating the backtrace if necessary. - let ret = match result { + let mut ret = match result { Ok(ret) => Ok(InvocationResult { exit_code: ExitCode::OK, value: ret.cloned(), @@ -851,7 +845,9 @@ where .price_list() .on_method_return(cm.call_stack_depth, ret_size) { - let _ = cm.charge_gas(charge); + if let Err(e) = cm.charge_gas(charge) { + ret = Err(e); + } } } diff --git a/fvm/src/gas/mod.rs b/fvm/src/gas/mod.rs index ad26acfe82..4b1c73bf85 100644 --- a/fvm/src/gas/mod.rs +++ b/fvm/src/gas/mod.rs @@ -298,9 +298,9 @@ mod tests { #[allow(clippy::identity_op)] fn basic_gas_tracker() -> Result<()> { let t = GasTracker::new(Gas::new(20), Gas::new(10), false); - let _ = t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?; + t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?; assert_eq!(t.gas_used(), Gas::new(15)); - let _ = t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?; + t.apply_charge(GasCharge::new("", Gas::new(5), Gas::zero()))?; assert_eq!(t.gas_used(), Gas::new(20)); assert!(t .apply_charge(GasCharge::new("", Gas::new(1), Gas::zero())) diff --git a/fvm/src/gas/timer.rs b/fvm/src/gas/timer.rs index 2f4f36c08d..3684cd0a08 100644 --- a/fvm/src/gas/timer.rs +++ b/fvm/src/gas/timer.rs @@ -27,7 +27,6 @@ pub type GasInstant = Instant; /// A handle returned by `charge_gas` which must be used to mark the end of /// the execution associated with that gas. -#[must_use] #[derive(Debug)] pub struct GasTimer(Option); diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 96ba5b3239..5c91a84642 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -264,8 +264,7 @@ where fn block_open(&mut self, cid: &Cid) -> Result<(BlockId, BlockStat)> { // TODO(M2): Check for reachability here. - let _ = self - .call_manager + self.call_manager .charge_gas(self.call_manager.price_list().on_block_open_base())?; let start = GasTimer::start(); @@ -737,9 +736,8 @@ where ); } - let _ = self - .call_manager - .charge_gas(self.call_manager.price_list().on_tipset_cid(offset > 1)); + self.call_manager + .charge_gas(self.call_manager.price_list().on_tipset_cid(offset > 1))?; self.call_manager.externs().get_tipset_cid(epoch).or_fatal() } diff --git a/fvm/src/syscalls/bind.rs b/fvm/src/syscalls/bind.rs index ec959339d4..5ebb2d765c 100644 --- a/fvm/src/syscalls/bind.rs +++ b/fvm/src/syscalls/bind.rs @@ -97,7 +97,7 @@ fn memory_and_data<'a, K: Kernel>( macro_rules! charge_syscall_gas { ($kernel:expr) => { let charge = $kernel.price_list().on_syscall(); - let _ = $kernel + $kernel .charge_gas(&charge.name, charge.compute_gas) .map_err(Abort::from_error_as_fatal)?; }; diff --git a/fvm/src/syscalls/mod.rs b/fvm/src/syscalls/mod.rs index 69a6063a5b..6f0b5aad0d 100644 --- a/fvm/src/syscalls/mod.rs +++ b/fvm/src/syscalls/mod.rs @@ -151,8 +151,7 @@ pub fn charge_for_exec( // Only recording time for the execution, not for the memory part, which is unknown. But we // could perform stomething like a multi-variate linear regression to see if the amount of // memory explains any of the exectuion time. - let _ = data - .kernel + data.kernel .charge_gas("wasm_memory_grow", memory_gas_charge) .map_err(Abort::from_error_as_fatal)?; } @@ -179,7 +178,7 @@ pub fn charge_for_init( if let Some(min_table_elements) = min_table_elements(module) { let table_gas = data.kernel.price_list().init_table_gas(min_table_elements); - let _ = data.kernel.charge_gas("wasm_table_init", table_gas)?; + data.kernel.charge_gas("wasm_table_init", table_gas)?; } data.kernel.charge_gas("wasm_memory_init", memory_gas)