Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighten up saturating_ math checks #1254

Merged
merged 4 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion soroban-env-host/src/budget/limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ impl DepthLimiter for BudgetImpl {
// `leave` should be called in tandem with `enter` such that the depth
// doesn't exceed the initial depth limit.
fn leave(&mut self) -> Result<(), HostError> {
self.depth_limit = self.depth_limit.saturating_add(1);
self.depth_limit = self.depth_limit.checked_add(1).ok_or_else(|| {
Error::from_type_and_code(ScErrorType::Context, ScErrorCode::InternalError)
})?;
Ok(())
}
}
Expand Down
27 changes: 19 additions & 8 deletions soroban-env-host/src/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,10 +954,13 @@ impl VmCallerEnv for Host {
let msg = String::from_utf8_lossy(&msg);

let VmSlice { vm, pos, len } = self.decode_vmslice(vals_pos, vals_len)?;
Vec::<Val>::charge_bulk_init_cpy(len.saturating_add(1) as u64, self)?;
Vec::<Val>::charge_bulk_init_cpy((len as u64).saturating_add(1), self)?;
let mut vals: Vec<Val> = vec![Val::VOID.to_val(); len as usize];
// charge for conversion from bytes to `Val`s
self.charge_budget(ContractCostType::MemCpy, Some(len.saturating_mul(8) as u64))?;
self.charge_budget(
ContractCostType::MemCpy,
Some((len as u64).saturating_mul(8)),
)?;
self.metered_vm_read_vals_from_linear_memory::<8, Val>(
vmcaller,
&vm,
Expand Down Expand Up @@ -1510,7 +1513,10 @@ impl VmCallerEnv for Host {
Vec::<Val>::charge_bulk_init_cpy(len as u64, self)?;
let mut vals: Vec<Val> = vec![Val::VOID.into(); len as usize];
// charge for conversion from bytes to `Val`s
self.charge_budget(ContractCostType::MemCpy, Some(len.saturating_mul(8) as u64))?;
self.charge_budget(
ContractCostType::MemCpy,
Some((len as u64).saturating_mul(8)),
)?;
self.metered_vm_read_vals_from_linear_memory::<8, Val>(
vmcaller,
&vm,
Expand Down Expand Up @@ -1581,7 +1587,7 @@ impl VmCallerEnv for Host {

// Step 2: write all vals.
// charges memcpy of converting map entries into bytes
self.charge_budget(ContractCostType::MemCpy, Some(len.saturating_mul(8) as u64))?;
self.charge_budget(ContractCostType::MemCpy, Some((len as u64).saturating_mul(8)))?;
self.metered_vm_write_vals_to_linear_memory(
vmcaller,
&vm,
Expand Down Expand Up @@ -1806,7 +1812,10 @@ impl VmCallerEnv for Host {
Vec::<Val>::charge_bulk_init_cpy(len as u64, self)?;
let mut vals: Vec<Val> = vec![Val::VOID.to_val(); len as usize];
// charge for conversion from bytes to `Val`s
self.charge_budget(ContractCostType::MemCpy, Some(len.saturating_mul(8) as u64))?;
self.charge_budget(
ContractCostType::MemCpy,
Some((len as u64).saturating_mul(8)),
)?;
self.metered_vm_read_vals_from_linear_memory::<8, Val>(
vmcaller,
&vm,
Expand Down Expand Up @@ -1838,7 +1847,7 @@ impl VmCallerEnv for Host {
));
}
// charges memcpy of converting vec entries into bytes
self.charge_budget(ContractCostType::MemCpy, Some(len.saturating_mul(8) as u64))?;
self.charge_budget(ContractCostType::MemCpy, Some((len as u64).saturating_mul(8)))?;
self.metered_vm_write_vals_to_linear_memory(
vmcaller,
&vm,
Expand Down Expand Up @@ -2416,8 +2425,10 @@ impl VmCallerEnv for Host {
let vnew = self.visit_obj(b, |hv: &ScBytes| {
self.validate_index_lt_bound(i, hv.len())?;
let mut vnew: Vec<u8> = hv.metered_clone(self)?.into();
// len > i has been verified above but use saturating_sub just in case
let n_elts = (hv.len() as u64).saturating_sub(i as u64);
// len > i has been verified above but use checked_sub just in case
let n_elts = (hv.len() as u64).checked_sub(i as u64).ok_or_else(|| {
Error::from_type_and_code(ScErrorType::Context, ScErrorCode::InternalError)
})?;
// remove elements incurs the cost of moving bytes, it does not incur
// allocation/deallocation
metered_clone::charge_shallow_copy::<u8>(n_elts, self)?;
Expand Down
35 changes: 28 additions & 7 deletions soroban-env-host/src/host/ledger_info_helper.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use soroban_env_common::xdr::{ContractDataDurability, LedgerKey};

use crate::{Host, HostError, LedgerInfo};
use crate::{
xdr::{ContractDataDurability, LedgerKey, ScErrorCode, ScErrorType},
Error, Host, HostError, LedgerInfo,
};

impl Host {
pub(crate) fn get_min_live_until_ledger(
Expand All @@ -16,16 +17,36 @@ impl Host {
self.with_ledger_info(|li: &LedgerInfo| Ok(li.min_persistent_entry_ttl))?
}
};
Ok(ledger_seq.saturating_add(min_live_until.saturating_sub(1)))
ledger_seq
.checked_add(min_live_until.saturating_sub(1))
.ok_or_else(|| {
// overflowing here means a misconfiguration of the network (the
// ttl is too large), in which case we immediately flag it as an
// unrecoverable `InternalError`, even though the source is
// external to the host.
HostError::from(Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InternalError,
))
})
}

pub(crate) fn max_live_until_ledger(&self) -> Result<u32, HostError> {
self.with_ledger_info(|li| {
Ok(li
.sequence_number
li.sequence_number
// Entry can live for at most max_entry_live_until ledgers from
// now, counting the current one.
.saturating_add(li.max_entry_ttl.saturating_sub(1)))
.checked_add(li.max_entry_ttl.saturating_sub(1))
dmkozh marked this conversation as resolved.
Show resolved Hide resolved
.ok_or_else(|| {
// overflowing here means a misconfiguration of the network
// (the ttl is too large), in which case we immediately flag
// it as an unrecoverable `InternalError`, even though the
// source is external to the host.
HostError::from(Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InternalError,
))
})
})
}
}
Expand Down
28 changes: 18 additions & 10 deletions soroban-env-host/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@

use std::rc::Rc;

use soroban_env_common::xdr::{ContractDataDurability, ScErrorCode, ScErrorType};
use soroban_env_common::{Env, Val};

use crate::budget::Budget;
use crate::host::ledger_info_helper::get_key_durability;
use crate::xdr::{LedgerEntry, LedgerKey};
use crate::Host;
use crate::{host::metered_map::MeteredOrdMap, HostError};
use crate::{
budget::Budget,
host::{ledger_info_helper::get_key_durability, metered_map::MeteredOrdMap},
xdr::{ContractDataDurability, LedgerEntry, LedgerKey, ScErrorCode, ScErrorType},
Env, Error, Host, HostError, Val,
};

pub type FootprintMap = MeteredOrdMap<Rc<LedgerKey>, AccessType, Budget>;
pub type EntryWithLiveUntil = (Rc<LedgerEntry>, Option<u32>);
Expand Down Expand Up @@ -422,8 +420,18 @@ impl Storage {
));
}

let mut new_live_until =
host.with_ledger_info(|li| Ok(li.sequence_number.saturating_add(extend_to)))?;
let mut new_live_until = host.with_ledger_info(|li| {
li.sequence_number.checked_add(extend_to).ok_or_else(|| {
// overflowing here means a misconfiguration of the network (the
// ttl is too large), in which case we immediately flag it as an
// unrecoverable `InternalError`, even though the source is
// external to the host.
HostError::from(Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InternalError,
))
})
})?;

if new_live_until > host.max_live_until_ledger()? {
if let Some(durability) = get_key_durability(&key) {
Expand Down