From c35c6ed73ead93c64fc7c9c8db12d54f878b29d0 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 15:29:34 -0700 Subject: [PATCH 1/3] Mop up a couple missed cases from PR #925 --- soroban-env-host/benches/common/measure.rs | 2 +- soroban-env-host/benches/common/util.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/soroban-env-host/benches/common/measure.rs b/soroban-env-host/benches/common/measure.rs index 4ebd6d495..43d6cdafb 100644 --- a/soroban-env-host/benches/common/measure.rs +++ b/soroban-env-host/benches/common/measure.rs @@ -397,7 +397,7 @@ where ), { let mut recycled_samples = Vec::with_capacity(samples.len()); - host.as_budget().reset_unlimited(); + host.as_budget().reset_unlimited().unwrap(); // start the cpu and mem measurement mem_tracker.0.store(0, Ordering::SeqCst); diff --git a/soroban-env-host/benches/common/util.rs b/soroban-env-host/benches/common/util.rs index 918e6cdec..ef2501faf 100644 --- a/soroban-env-host/benches/common/util.rs +++ b/soroban-env-host/benches/common/util.rs @@ -2,8 +2,8 @@ use soroban_env_host::{budget::AsBudget, Host, Val, I256}; pub(crate) fn test_host() -> Host { let host = Host::default(); - host.as_budget().reset_unlimited(); - host.as_budget().reset_fuel_config(); + host.as_budget().reset_unlimited().unwrap(); + host.as_budget().reset_fuel_config().unwrap(); host } From 56f0db7720861ca814744c365675a84d2d1a2965 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 15:29:47 -0700 Subject: [PATCH 2/3] Cap vector and map sizes to u32::max --- .../benches/common/cost_types/vec_ops.rs | 2 +- soroban-env-host/src/host.rs | 9 +++++---- soroban-env-host/src/host/conversion.rs | 2 +- soroban-env-host/src/host/metered_map.rs | 3 +++ soroban-env-host/src/host/metered_vector.rs | 17 ++++++++++------- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/soroban-env-host/benches/common/cost_types/vec_ops.rs b/soroban-env-host/benches/common/cost_types/vec_ops.rs index bb8e548fc..0d2fc47dc 100644 --- a/soroban-env-host/benches/common/cost_types/vec_ops.rs +++ b/soroban-env-host/benches/common/cost_types/vec_ops.rs @@ -18,7 +18,7 @@ impl HostCostMeasurement for VecEntryMeasure { fn new_random_case(_host: &Host, rng: &mut StdRng, input: u64) -> VecEntrySample { let input = 1 + input * Self::STEP_SIZE; let ov = util::to_rawval_u32(0..(input as u32)).collect(); - let vec: MeteredVector<_> = MeteredVector::from_vec(ov); + let vec: MeteredVector<_> = MeteredVector::from_vec(ov).unwrap(); let mut idxs: Vec = (0..input as usize).collect(); idxs.shuffle(rng); VecEntrySample { vec, idxs } diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index cf7061854..3e06dadc0 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -1724,12 +1724,13 @@ impl VmCallerEnv for Host { } fn vec_new(&self, _vmcaller: &mut VmCaller, c: Val) -> Result { - let capacity: usize = if c.is_void() { + // NB: we ignore capacity because vectors are immutable + // and there's no reuse, we always size them exactly. + let _capacity: usize = if c.is_void() { 0 } else { self.usize_from_rawval_u32_input("c", c)? }; - // TODO: optimize the vector based on capacity self.add_host_object(HostVec::new()) } @@ -1941,7 +1942,7 @@ impl VmCallerEnv for Host { vals.as_mut_slice(), |buf| Val::from_payload(u64::from_le_bytes(*buf)), )?; - self.add_host_object(HostVec::from_vec(vals)) + self.add_host_object(HostVec::from_vec(vals)?) } fn vec_unpack_to_linear_memory( @@ -2824,7 +2825,7 @@ impl VmCallerEnv for Host { let inner = MeteredVector::from_array(&vals, self.as_budget())?; outer.push(self.add_host_object(inner)?.into()); } - self.add_host_object(HostVec::from_vec(outer)) + self.add_host_object(HostVec::from_vec(outer)?) } fn fail_with_error( diff --git a/soroban-env-host/src/host/conversion.rs b/soroban-env-host/src/host/conversion.rs index 99b73131c..58acda121 100644 --- a/soroban-env-host/src/host/conversion.rs +++ b/soroban-env-host/src/host/conversion.rs @@ -484,7 +484,7 @@ impl Host { for e in v.iter() { vv.push(self.to_host_val(e)?) } - Ok(self.add_host_object(HostVec::from_vec(vv))?.into()) + Ok(self.add_host_object(HostVec::from_vec(vv)?)?.into()) } ScVal::Map(Some(m)) => { metered_clone::charge_heap_alloc::<(Val, Val)>(m.len() as u64, self.as_budget())?; diff --git a/soroban-env-host/src/host/metered_map.rs b/soroban-env-host/src/host/metered_map.rs index 9747a75d7..223bcb902 100644 --- a/soroban-env-host/src/host/metered_map.rs +++ b/soroban-env-host/src/host/metered_map.rs @@ -84,6 +84,9 @@ where } pub fn from_map(map: Vec<(K, V)>, ctx: &Ctx) -> Result { + if u32::try_from(map.len()).is_err() { + return Err((ScErrorType::Object, ScErrorCode::ExceededLimit).into()); + } // Allocation cost already paid for by caller, here just checks that input // has sorted and unique keys. let m = MeteredOrdMap { diff --git a/soroban-env-host/src/host/metered_vector.rs b/soroban-env-host/src/host/metered_vector.rs index 5f1a2bee3..057196351 100644 --- a/soroban-env-host/src/host/metered_vector.rs +++ b/soroban-env-host/src/host/metered_vector.rs @@ -50,7 +50,7 @@ where { // Constructs a empty new `MeteredVector`. pub fn new() -> Self { - Self::from_vec(Vec::new()) + Self { vec: Vec::new() } } // Constructs a new, empty `MeteredVector` with at least the specified capacity. @@ -60,19 +60,22 @@ where #[cfg(any(test, feature = "testutils"))] pub fn with_capacity(capacity: usize, budget: &Budget) -> Result { super::metered_clone::charge_heap_alloc::(capacity as u64, budget)?; - Ok(Self::from_vec(Vec::with_capacity(capacity))) + Self::from_vec(Vec::with_capacity(capacity)) } pub fn from_array(buf: &[A], budget: &Budget) -> Result { // we may temporarily go over budget here. let vec: Vec = buf.into(); vec.charge_deep_clone(budget)?; - Ok(Self::from_vec(vec)) + Self::from_vec(vec) } // No meter charge, assuming allocation cost has been covered by the caller from the outside. - pub fn from_vec(vec: Vec) -> Self { - Self { vec } + pub fn from_vec(vec: Vec) -> Result { + if u32::try_from(vec.len()).is_err() { + return Err((ScErrorType::Object, ScErrorCode::ExceededLimit).into()); + } + Ok(Self { vec }) } pub fn as_slice(&self) -> &[A] { @@ -98,7 +101,7 @@ where // the clone into one (when A::IS_SHALLOW==true). let vec: Vec = iter.collect(); vec.charge_deep_clone(budget)?; - Ok(Self::from_vec(vec)) + Self::from_vec(vec) } else { // This is a logic error, we should never get here. Err((ScErrorType::Object, ScErrorCode::InternalError).into()) @@ -289,7 +292,7 @@ where } } vec.charge_deep_clone(budget)?; - Ok(Self::from_vec(vec)) + Self::from_vec(vec) } pub fn iter(&self) -> std::slice::Iter<'_, A> { From a23222b5da86786a7120791a66bcf466729d2066 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 16:37:51 -0700 Subject: [PATCH 3/3] Cast usize to u64 before multiplying, for platform-consistency. --- soroban-env-host/src/host/mem_helper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/soroban-env-host/src/host/mem_helper.rs b/soroban-env-host/src/host/mem_helper.rs index a25e3f7e3..97d19b029 100644 --- a/soroban-env-host/src/host/mem_helper.rs +++ b/soroban-env-host/src/host/mem_helper.rs @@ -167,7 +167,7 @@ impl Host { let mem_data = vm.get_memory(self)?.data(vmcaller.try_mut()?); self.charge_budget( ContractCostType::VmMemRead, - Some(num_slices.saturating_mul(8) as u64), + Some((num_slices as u64).saturating_mul(8)), )?; for i in 0..num_slices {