Repository: stellar/rs-soroban-env
Pull request #279 (2022-07-27 20:55:41)

Refactor index validity-checking code into a new file 
### What

Mostly just refactoring/clean-ups. Along the way fixed a couple of things. See comments. 
See commit log for details. 
(Note: the first commit d174f64 says "Bugfix" but it was not actually a bug. Just forgot to edit the commit message.)

### Why

[TODO: Why this change is being made. Include any context required to understand the why.]

### Known limitations

[TODO or N/A]


jayz22: Ready to be merged. Fixed the comment and reconciled with upstream renaming changes. 

Patch:
diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs
index 91068a03..7327e990 100644
--- a/soroban-env-host/src/host.rs
+++ b/soroban-env-host/src/host.rs
@@ -10,7 +10,7 @@ use num_integer::Integer;
 use num_traits::cast::ToPrimitive;
 use num_traits::{Pow, Signed, Zero};
 use soroban_env_common::{EnvVal, TryConvert, TryFromVal, TryIntoVal};
-use std::ops::{Add, BitAnd, BitOr, BitXor, Div, Mul, Neg, Not, Rem, Shl, Shr, Sub};
+use std::ops::{Add, BitAnd, BitOr, BitXor, Mul, Neg, Not, Rem, Shl, Shr, Sub};
 
 use soroban_env_common::xdr::{
     AccountEntry, AccountId, Hash, PublicKey, ReadXdr, ThresholdIndexes, WriteXdr,
@@ -41,6 +41,7 @@ use crate::{EnvBase, IntoVal, Object, RawVal, RawValConvertible, Symbol, Val, UN
 mod conversion;
 mod err_helper;
 mod error;
+mod validity;
 pub use error::HostError;
 
 /// Saves host state (storage and objects) for rolling back a (sub-)transaction
@@ -1057,15 +1058,12 @@ impl CheckedEnv for Host {
     }
 
     fn vec_put(&self, v: Object, i: RawVal, x: RawVal) -> Result<Object, HostError> {
-        let i: usize = self.usize_from_rawval_u32_input("i", i)?;
+        let i = self.u32_from_rawval_input("i", i)?;
         let x = self.associate_raw_val(x);
         let vnew = self.visit_obj(v, move |hv: &HostVec| {
-            if i >= hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
-            }
+            self.validate_index_lt_bound(i, hv.len())?;
             let mut vnew = hv.clone();
-            vnew.set(i, x);
+            vnew.set(i as usize, x);
             Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
@@ -1081,14 +1079,11 @@ impl CheckedEnv for Host {
     }
 
     fn vec_del(&self, v: Object, i: RawVal) -> Result<Object, HostError> {
-        let i: usize = self.usize_from_rawval_u32_input("i", i)?;
+        let i = self.u32_from_rawval_input("i", i)?;
         let vnew = self.visit_obj(v, move |hv: &HostVec| {
-            if i >= hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
-            }
+            self.validate_index_lt_bound(i, hv.len())?;
             let mut vnew = hv.clone();
-            vnew.remove(i);
+            vnew.remove(i as usize);
             Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
@@ -1136,15 +1131,12 @@ impl CheckedEnv for Host {
     }
 
     fn vec_insert(&self, v: Object, i: RawVal, x: RawVal) -> Result<Object, HostError> {
-        let i = self.usize_from_rawval_u32_input("i", i)?;
+        let i = self.u32_from_rawval_input("i", i)?;
         let x = self.associate_raw_val(x);
         let vnew = self.visit_obj(v, move |hv: &HostVec| {
-            if i > hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
-            }
+            self.validate_index_le_bound(i, hv.len())?;
             let mut vnew = hv.clone();
-            vnew.insert(i, x);
+            vnew.insert(i as usize, x);
             Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
@@ -1161,26 +1153,11 @@ impl CheckedEnv for Host {
     }
 
     fn vec_slice(&self, v: Object, start: RawVal, end: RawVal) -> Result<Object, HostError> {
-        let start = self.usize_from_rawval_u32_input("start", start)?;
-        let end = self.usize_from_rawval_u32_input("end", end)?;
-        if start > end {
-            return Err(self.err_status_msg(
-                ScHostFnErrorCode::InputArgsInvalid,
-                "start greater than end",
-            ));
-        }
+        let start = self.u32_from_rawval_input("start", start)?;
+        let end = self.u32_from_rawval_input("end", end)?;
         let vnew = self.visit_obj(v, move |hv: &HostVec| {
-            if start > hv.len() {
-                return Err(self.err_status_msg(
-                    ScHostObjErrorCode::VecIndexOutOfBound,
-                    "start out of bounds",
-                ));
-            }
-            if end > hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "end out of bounds"));
-            }
-            Ok(hv.clone().slice(start..end))
+            self.validate_start_end_bound(start, end, hv.len())?;
+            Ok(hv.clone().slice(start as usize..end as usize))
         })?;
         Ok(self.add_host_object(vnew)?.into())
     }
@@ -1352,10 +1329,8 @@ impl CheckedEnv for Host {
     fn bigint_div(&self, x: Object, y: Object) -> Result<Object, HostError> {
         let res = self.visit_obj(x, |a: &BigInt| {
             self.visit_obj(y, |b: &BigInt| {
-                if b.is_zero() {
-                    return Err(self.err_general("bigint division by zero"));
-                }
-                Ok(a.div(b))
+                a.checked_div(b)
+                    .ok_or_else(|| self.err_general("bigint division by zero"))
             })
         })?;
         Ok(self.add_host_object(res)?.into())
@@ -1544,15 +1519,7 @@ impl CheckedEnv for Host {
             let VmSlice { vm, pos, len } = self.decode_vmslice(lm_pos, len)?;
             let b_pos = u32::try_from(b_pos)?;
             self.visit_obj(b, move |hv: &Vec<u8>| {
-                let end_idx = b_pos.checked_add(len).ok_or_else(|| {
-                    self.err_status_msg(ScHostFnErrorCode::InputArgsInvalid, "u32 overflow")
-                })? as usize;
-                if end_idx > hv.len() {
-                    return Err(self.err_status_msg(
-                        ScHostObjErrorCode::VecIndexOutOfBound,
-                        "index out of bound",
-                    ));
-                }
+                let end_idx = self.validate_start_span_bound(b_pos, len, hv.len())? as usize;
                 vm.with_memory_access(self, |mem| {
                     self.map_err(mem.set(pos, &hv.as_slice()[b_pos as usize..end_idx]))
                 })?;
@@ -1619,12 +1586,13 @@ impl CheckedEnv for Host {
         let u = self.u8_from_rawval_input("u", u)?;
         let vnew = self.visit_obj(b, move |hv: &Vec<u8>| {
             let mut vnew = hv.clone();
-            if i >= hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
+            match vnew.get_mut(i) {
+                None => Err(self.err_status(ScHostObjErrorCode::VecIndexOutOfBound)),
+                Some(v) => {
+                    *v = u;
+                    Ok(vnew)
+                }
             }
-            let _old_val = std::mem::replace(&mut vnew[i], u);
-            Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
     }
@@ -1639,14 +1607,11 @@ impl CheckedEnv for Host {
     }
 
     fn binary_del(&self, b: Object, i: RawVal) -> Result<Object, HostError> {
-        let i = self.usize_from_rawval_u32_input("i", i)?;
+        let i = self.u32_from_rawval_input("i", i)?;
         let vnew = self.visit_obj(b, move |hv: &Vec<u8>| {
-            if i >= hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
-            }
+            self.validate_index_lt_bound(i, hv.len())?;
             let mut vnew = hv.clone();
-            vnew.remove(i);
+            vnew.remove(i as usize);
             Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
@@ -1694,15 +1659,12 @@ impl CheckedEnv for Host {
     }
 
     fn binary_insert(&self, b: Object, i: RawVal, u: RawVal) -> Result<Object, HostError> {
-        let i = self.usize_from_rawval_u32_input("i", i)?;
+        let i = self.u32_from_rawval_input("i", i)?;
         let u = self.u8_from_rawval_input("u", u)?;
         let vnew = self.visit_obj(b, move |hv: &Vec<u8>| {
-            if i > hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "index out of bound"));
-            }
+            self.validate_index_le_bound(i, hv.len())?;
             let mut vnew = hv.clone();
-            vnew.insert(i, u);
+            vnew.insert(i as usize, u);
             Ok(vnew)
         })?;
         Ok(self.add_host_object(vnew)?.into())
@@ -1719,20 +1681,11 @@ impl CheckedEnv for Host {
     }
 
     fn binary_slice(&self, b: Object, start: RawVal, end: RawVal) -> Result<Object, HostError> {
-        let start = self.usize_from_rawval_u32_input("start", start)?;
-        let end = self.usize_from_rawval_u32_input("end", end)?;
+        let start = self.u32_from_rawval_input("start", start)?;
+        let end = self.u32_from_rawval_input("end", end)?;
         let vnew = self.visit_obj(b, move |hv: &Vec<u8>| {
-            if start > hv.len() {
-                return Err(self.err_status_msg(
-                    ScHostObjErrorCode::VecIndexOutOfBound,
-                    "start out of bounds",
-                ));
-            }
-            if end > hv.len() {
-                return Err(self
-                    .err_status_msg(ScHostObjErrorCode::VecIndexOutOfBound, "end out of bounds"));
-            }
-            Ok(hv.as_slice()[start..end].to_vec())
+            self.validate_start_end_bound(start, end, hv.len())?;
+            Ok(hv.as_slice()[start as usize..end as usize].to_vec())
         })?;
         Ok(self.add_host_object(vnew)?.into())
     }
diff --git a/soroban-env-host/src/host/validity.rs b/soroban-env-host/src/host/validity.rs
new file mode 100644
index 00000000..8af6ae18
--- /dev/null
+++ b/soroban-env-host/src/host/validity.rs
@@ -0,0 +1,72 @@
+use crate::xdr::{ScHostFnErrorCode, ScHostObjErrorCode};
+use crate::{Host, HostError};
+
+impl Host {
+    pub(crate) fn validate_index_lt_bound(
+        &self,
+        index: u32,
+        bound: usize,
+    ) -> Result<(), HostError> {
+        if index as usize >= bound {
+            return Err(self.err_status_msg(
+                ScHostObjErrorCode::VecIndexOutOfBound, // TODO: need to reconcile between InputArgsInvalid and VecIndexOutOfBound
+                "start index out of bound",
+            ));
+        }
+        Ok(())
+    }
+
+    pub(crate) fn validate_index_le_bound(
+        &self,
+        index: u32,
+        bound: usize,
+    ) -> Result<(), HostError> {
+        if index as usize > bound {
+            return Err(self.err_status_msg(
+                ScHostObjErrorCode::VecIndexOutOfBound, // TODO: need to reconcile between InputArgsInvalid and VecIndexOutOfBound
+                "start index out of bound",
+            ));
+        }
+        Ok(())
+    }
+
+    pub(crate) fn validate_start_end_bound(
+        &self,
+        start: u32,
+        end: u32,
+        bound: usize,
+    ) -> Result<(), HostError> {
+        if start as usize >= bound {
+            return Err(self.err_status_msg(
+                ScHostObjErrorCode::VecIndexOutOfBound,
+                "start index out of bound",
+            ));
+        }
+        if end as usize > bound {
+            return Err(self.err_status_msg(
+                ScHostObjErrorCode::VecIndexOutOfBound,
+                "end index out of bound",
+            ));
+        }
+        if start > end {
+            return Err(self.err_status_msg(
+                ScHostFnErrorCode::InputArgsInvalid,
+                "start greater than end",
+            ));
+        }
+        Ok(())
+    }
+
+    pub(crate) fn validate_start_span_bound(
+        &self,
+        start: u32,
+        span: u32,
+        bound: usize,
+    ) -> Result<u32, HostError> {
+        let end = start.checked_add(span).ok_or_else(|| {
+            self.err_status_msg(ScHostFnErrorCode::InputArgsInvalid, "u32 overflow")
+        })?;
+        self.validate_start_end_bound(start, end, bound)?;
+        Ok(end)
+    }
+}


Test patch:
diff --git a/soroban-env-host/src/test/binary.rs b/soroban-env-host/src/test/binary.rs
index b4877c2d..82a3f54a 100644
--- a/soroban-env-host/src/test/binary.rs
+++ b/soroban-env-host/src/test/binary.rs
@@ -1,7 +1,9 @@
+use crate::xdr::ScHostFnErrorCode;
 use crate::{
     xdr::{ScHostObjErrorCode, ScObject, ScStatic, ScStatus, ScVal},
     CheckedEnv, Host, HostError, RawVal, RawValConvertible,
 };
+use soroban_env_common::EnvBase;
 
 #[cfg(feature = "vm")]
 use super::wasm_examples::LINEAR_MEMORY;
@@ -84,6 +86,26 @@ fn binary_suite_of_tests() -> Result<(), HostError> {
     Ok(())
 }
 
+#[test]
+fn binary_put_out_of_bound() -> Result<(), HostError> {
+    let host = Host::default();
+    let obj = host.binary_new()?;
+    let res = host.binary_put(obj, 0u32.into(), 1u32.into());
+    let code = ScHostObjErrorCode::VecIndexOutOfBound;
+    assert!(HostError::result_matches_err_status(res, code));
+    Ok(())
+}
+
+#[test]
+fn binary_slice_start_greater_than_end() -> Result<(), HostError> {
+    let host = Host::default();
+    let obj = host.binary_new_from_slice(&[1, 2, 3, 4]);
+    let res = host.binary_slice(obj, 2_u32.into(), 1_u32.into());
+    let code = ScHostFnErrorCode::InputArgsInvalid;
+    assert!(HostError::result_matches_err_status(res, code));
+    Ok(())
+}
+
 #[test]
 fn binary_xdr_roundtrip() -> Result<(), HostError> {
     let host = Host::default();