Repository: stellar/rs-soroban-env
Pull request #294 (2022-07-30 10:46:36)

Revert "Revert "Refactor index validity-checking code into a new file ""
### What
Reopens https://github.com/stellar/rs-soroban-env/pull/291
- Reverts https://github.com/stellar/rs-soroban-env/pull/290, which reverted https://github.com/stellar/rs-soroban-env/pull/279. Bring back the index validity checking code.
- Adds the fix, improves interface, adds tests

### Why

[TODO: Why this change is being made. Include any context required to understand the why.]

### Known limitations

[TODO or N/A]


leighmcculloch: I tested this against the SDK and the SDK tests pass.

Patch:
diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs
index 91068a03..c2e95035 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))
+            let range = self.valid_range_from_start_end_bound(start, end, hv.len())?;
+            Ok(hv.clone().slice(range))
         })?;
         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,17 +1519,9 @@ 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 range = self.valid_range_from_start_span_bound(b_pos, len, hv.len())?;
                 vm.with_memory_access(self, |mem| {
-                    self.map_err(mem.set(pos, &hv.as_slice()[b_pos as usize..end_idx]))
+                    self.map_err(mem.set(pos, &hv.as_slice()[range]))
                 })?;
                 Ok(().into())
             })
@@ -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())
+            let range = self.valid_range_from_start_end_bound(start, end, hv.len())?;
+            Ok(hv.as_slice()[range].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..307cc2d7
--- /dev/null
+++ b/soroban-env-host/src/host/validity.rs
@@ -0,0 +1,79 @@
+use std::ops::Range;
+
+use crate::events::DebugError;
+use crate::xdr::{ScHostFnErrorCode, ScHostObjErrorCode};
+use crate::{Host, HostError, RawVal};
+
+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 valid_range_from_start_end_bound(
+        &self,
+        start: u32,
+        end: u32,
+        bound: usize,
+    ) -> Result<Range<usize>, 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(
+                DebugError::new(ScHostFnErrorCode::InputArgsInvalid)
+                    .msg("index starts at {} but ends at {}")
+                    .arg(RawVal::from_u32(start))
+                    .arg(RawVal::from_u32(end)),
+            ));
+        }
+        Ok(Range {
+            start: start as usize,
+            end: end as usize,
+        })
+    }
+
+    pub(crate) fn valid_range_from_start_span_bound(
+        &self,
+        start: u32,
+        span: u32,
+        bound: usize,
+    ) -> Result<Range<usize>, HostError> {
+        let end = start.checked_add(span).ok_or_else(|| {
+            self.err_status_msg(ScHostFnErrorCode::InputArgsInvalid, "u32 overflow")
+        })?;
+        self.valid_range_from_start_end_bound(start, end, bound)
+    }
+}


Test patch:
diff --git a/soroban-env-host/src/test/binary.rs b/soroban-env-host/src/test/binary.rs
index b4877c2d..6d9162e9 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,45 @@ 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_slice_start_equal_len() -> Result<(), HostError> {
+    let host = Host::default();
+    let obj = host.binary_new_from_slice(&[1, 2, 3, 4]);
+    let res = host.binary_slice(obj, 4_u32.into(), 4_u32.into())?;
+    assert_eq!(host.obj_cmp(res.into(), host.binary_new()?.into())?, 0);
+    Ok(())
+}
+
+#[test]
+fn binary_slice_start_greater_than_len() -> Result<(), HostError> {
+    let host = Host::default();
+    let obj = host.binary_new_from_slice(&[1, 2, 3, 4]);
+    let res = host.binary_slice(obj, 5_u32.into(), 10_u32.into());
+    let code = ScHostObjErrorCode::VecIndexOutOfBound;
+    assert!(HostError::result_matches_err_status(res, code));
+    Ok(())
+}
+
 #[test]
 fn binary_xdr_roundtrip() -> Result<(), HostError> {
     let host = Host::default();