More code review and cleanup of host.rs module (#1088)

Today I didn't get as much done -- had some errands and a doctor
appointment -- but my goal was to get through the residual bits of
host.rs and I did that at least. Along the way I removed the `allows`
that enabled un-warned dead code and unused variables, and found some
stuff:

- Two missing error checks on linear-memory "unpack" vector lengths.
These don't represent a _major_ vulnerability, but they do allow users
to do an unpack to a mismatched vector length, which could hide a bug. A
footgun at least. Fixed.
- A dead cost type, based on a previous iteration of the secp256k1
interface. Filed #1087 to track removing it.
- Some diagnositcs weren't using the names we so meticulously pass down
to them. Just throwing them out. Fixed.
  - There was a bunch of dead code and over-public interfaces. Fixed.

.github/workflows/rust.yml

@@ -83,9 +83,9 @@ jobs:
       with:
         name: cargo-hack
         version: 0.5.16
-    - run: cargo hack --feature-powerset clippy --locked --target ${{ matrix.sys.target }}
+    - run: cargo hack --each-feature clippy --locked --target ${{ matrix.sys.target }}
     - if: matrix.sys.test
-      run: cargo hack --feature-powerset test --locked --target ${{ matrix.sys.target }}
+      run: cargo hack --each-feature test --locked --target ${{ matrix.sys.target }}
 
   publish-dry-run:
     if: github.event_name == 'push' || startsWith(github.head_ref, 'release/')

Makefile

@@ -1,10 +1,10 @@
 all: build test
 
 test:
-	cargo hack --feature-powerset test
+	cargo hack --each-feature test
 
 build:
-	cargo hack --feature-powerset clippy
+	cargo hack --each-feature clippy
 
 watch:
 	cargo watch --clear --watch-when-idle --shell '$(MAKE)'

publish-dry-run.sh

@@ -15,7 +15,7 @@ mv Cargo.toml.bak Cargo.toml
 
 # Package the crates that will be published. Verification is disabled because
 # we aren't ready to verify yet.
-cargo-hack hack --ignore-private package --no-verify --feature-powerset
+cargo-hack hack --ignore-private package --no-verify --each-feature
 
 # Add each crate that was packaged to the vendor/ directory.
 for crate in target/package/*.crate

soroban-env-host/src/events/system_events.rs

@@ -9,10 +9,10 @@ impl Host {
         Ok(())
     }
 
-    // Emits a system event for updating the contract executable.
-    // The only event topic is "executable_update" and the data contains
-    // a vector of [old_executable, new_executable] encoded as contract types
-    // (`ContractExecutable` above).
+    // Emits a system event for updating the contract executable. The topic
+    // vector contains the symbol "executable_update" followed by the
+    // old_executable and new_executable, encoded as contract types
+    // (`ContractExecutable` above). The event data is empty.
     pub(crate) fn emit_update_contract_event(
         &self,
         old_executable: &xdr::ContractExecutable,

soroban-env-host/src/host.rs

@@ -1,6 +1,3 @@
-#![allow(unused_variables)]
-#![allow(dead_code)]
-
 use core::{cell::RefCell, cmp::Ordering, fmt::Debug};
 use std::rc::Rc;
 
@@ -9,22 +6,22 @@ use crate::{
     budget::{AsBudget, Budget},
     events::{diagnostic::DiagnosticLevel, Events, InternalEventsBuffer},
     host_object::{HostMap, HostObject, HostObjectType, HostVec},
-    impl_bignum_host_fns_rhs_u32, impl_wrapping_obj_from_num, impl_wrapping_obj_to_num,
+    impl_bignum_host_fns, impl_bignum_host_fns_rhs_u32, impl_wrapping_obj_from_num,
+    impl_wrapping_obj_to_num,
     num::*,
     storage::Storage,
     xdr::{
         int128_helpers, AccountId, Asset, ContractCostType, ContractEventType, ContractExecutable,
-        CreateContractArgs, Duration, Hash, LedgerEntryData, PublicKey, ScAddress, ScBytes,
-        ScErrorType, ScString, ScSymbol, ScVal, TimePoint,
+        ContractIdPreimage, ContractIdPreimageFromAddress, CreateContractArgs, Duration, Hash,
+        LedgerEntryData, PublicKey, ScAddress, ScBytes, ScErrorCode, ScErrorType, ScString,
+        ScSymbol, ScVal, TimePoint, Uint256,
     },
-    AddressObject, Bool, BytesObject, ConversionError, Error, I128Object, I256Object, MapObject,
-    StorageType, StringObject, SymbolObject, SymbolSmall, SymbolStr, TryFromVal, U128Object,
-    U256Object, U32Val, U64Val, VecObject, VmCaller, VmCallerEnv, Void, I256, U256,
+    AddressObject, Bool, BytesObject, Compare, ConversionError, EnvBase, Error, I128Object,
+    I256Object, MapObject, Object, StorageType, StringObject, Symbol, SymbolObject, SymbolSmall,
+    TryFromVal, U128Object, U256Object, U32Val, U64Val, Val, VecObject, Vm, VmCaller, VmCallerEnv,
+    Void, I256, U256,
 };
 
-use crate::Vm;
-use crate::{EnvBase, Object, Symbol, Val};
-
 mod comparison;
 mod conversion;
 pub(crate) mod crypto;
@@ -41,23 +38,18 @@ pub(crate) mod metered_vector;
 pub(crate) mod metered_xdr;
 mod num;
 mod prng;
-pub use prng::{Seed, SEED_BYTES};
 mod validity;
+
 pub use error::HostError;
-use soroban_env_common::xdr::{
-    ContractIdPreimage, ContractIdPreimageFromAddress, ScErrorCode, Uint256,
-};
+pub use prng::{Seed, SEED_BYTES};
 
 use self::{
     frame::{Context, ContractReentryMode},
-    prng::Prng,
-};
-use self::{
     metered_clone::{MeteredClone, MeteredContainer},
     metered_xdr::metered_write_xdr,
+    prng::Prng,
 };
-use crate::impl_bignum_host_fns;
-use crate::Compare;
+
 #[cfg(any(test, feature = "testutils"))]
 pub use frame::ContractFunctionSet;
 pub(crate) use frame::Frame;
@@ -144,13 +136,15 @@ impl Default for Host {
 macro_rules! impl_checked_borrow_helpers {
     ($field:ident, $t:ty, $borrow:ident, $borrow_mut:ident) => {
         impl Host {
+            #[allow(dead_code)]
             pub(crate) fn $borrow(&self) -> Result<std::cell::Ref<'_, $t>, HostError> {
                 use crate::host::error::TryBorrowOrErr;
                 self.0.$field.try_borrow_or_err_with(
                     self,
                     concat!("host.0.", stringify!($field), ".try_borrow failed"),
                 )
             }
+            #[allow(dead_code)]
             pub(crate) fn $borrow_mut(&self) -> Result<std::cell::RefMut<'_, $t>, HostError> {
                 use crate::host::error::TryBorrowOrErr;
                 self.0.$field.try_borrow_mut_or_err_with(
@@ -350,16 +344,6 @@ impl Host {
         self.with_ledger_info(|li| Ok(li.protocol_version))
     }
 
-    /// Helper for mutating the [`Budget`] held in this [`Host`], either to
-    /// allocate it on contract creation or to deplete it on callbacks from
-    /// the VM or host functions.
-    pub(crate) fn with_budget<T, F>(&self, f: F) -> Result<T, HostError>
-    where
-        F: FnOnce(Budget) -> Result<T, HostError>,
-    {
-        f(self.0.budget.clone())
-    }
-
     pub(crate) fn budget_ref(&self) -> &Budget {
         &self.0.budget
     }
@@ -369,7 +353,7 @@ impl Host {
     }
 
     pub fn charge_budget(&self, ty: ContractCostType, input: Option<u64>) -> Result<(), HostError> {
-        self.0.budget.clone().charge(ty, input)
+        self.0.budget.charge(ty, input)
     }
 
     /// Accept a _unique_ (refcount = 1) host reference and destroy the
@@ -386,43 +370,6 @@ impl Host {
                 Error::from_type_and_code(ScErrorType::Context, ScErrorCode::InternalError).into()
             })
     }
-
-    // Testing interface to create values directly for later use via Env functions.
-    // It needs to be a `pub` method because benches are considered a separate crate.
-    #[cfg(any(test, feature = "testutils"))]
-    pub fn inject_val(&self, v: &ScVal) -> Result<Val, HostError> {
-        self.to_host_val(v).map(Into::into)
-    }
-
-    fn symbol_matches(&self, s: &[u8], sym: Symbol) -> Result<bool, HostError> {
-        if let Ok(ss) = SymbolSmall::try_from(sym) {
-            let sstr: SymbolStr = ss.into();
-            let slice: &[u8] = sstr.as_ref();
-            self.as_budget()
-                .compare(&slice, &s)
-                .map(|c| c == Ordering::Equal)
-        } else {
-            let sobj: SymbolObject = sym.try_into()?;
-            self.visit_obj(sobj, |scsym: &ScSymbol| {
-                self.as_budget()
-                    .compare(&scsym.as_slice(), &s)
-                    .map(|c| c == Ordering::Equal)
-            })
-        }
-    }
-
-    fn check_symbol_matches(&self, s: &[u8], sym: Symbol) -> Result<(), HostError> {
-        if self.symbol_matches(s, sym)? {
-            Ok(())
-        } else {
-            Err(self.err(
-                ScErrorType::Value,
-                ScErrorCode::InvalidInput,
-                "symbol mismatch",
-                &[sym.to_val()],
-            ))
-        }
-    }
 }
 
 // Notes on metering: these are called from the guest and thus charged on the VM instructions.
@@ -1245,7 +1192,7 @@ impl VmCallerEnv for Host {
             &vm,
             keys_pos,
             len as usize,
-            |n, slice| {
+            |_n, slice| {
                 self.charge_budget(ContractCostType::VmMemRead, Some(slice.len() as u64))?;
                 let scsym = ScSymbol(slice.try_into()?);
                 let sym = Symbol::try_from(self.to_host_val(&ScVal::Symbol(scsym))?)?;
@@ -1292,6 +1239,14 @@ impl VmCallerEnv for Host {
             len,
         } = self.decode_vmslice(keys_pos, len)?;
         self.visit_obj(map, |mapobj: &HostMap| {
+            if mapobj.len() != len as usize {
+                return Err(self.err(
+                    ScErrorType::Object,
+                    ScErrorCode::UnexpectedSize,
+                    "differing host map and output slice lengths when unpacking map to linear memory",
+                    &[],
+                ));
+            }
             // Step 1: check all key symbols.
             self.metered_vm_scan_slices_in_linear_memory(
                 vmcaller,
@@ -1571,10 +1526,18 @@ impl VmCallerEnv for Host {
     ) -> Result<Void, HostError> {
         let VmSlice { vm, pos, len } = self.decode_vmslice(vals_pos, len)?;
         self.visit_obj(vec, |vecobj: &HostVec| {
+            if vecobj.len() != len as usize {
+                return Err(self.err(
+                    ScErrorType::Object,
+                    ScErrorCode::UnexpectedSize,
+                    "differing host vector and output vector lengths when unpacking vec to linear memory",
+                    &[],
+                ));
+            }
             self.metered_vm_write_vals_to_linear_memory(
                 vmcaller,
                 &vm,
-                vals_pos.into(),
+                pos,
                 vecobj.as_slice(),
                 |x| {
                     Ok(u64::to_le_bytes(
@@ -2618,6 +2581,26 @@ impl VmCallerEnv for Host {
     // endregion "prng" module functions
 }
 
+#[cfg(any(test, feature = "testutils"))]
+impl Host {
+    // Testing interface to create values directly for later use via Env functions.
+    // It needs to be a `pub` method because benches are considered a separate crate.
+    pub fn inject_val(&self, v: &ScVal) -> Result<Val, HostError> {
+        self.to_host_val(v).map(Into::into)
+    }
+
+    /// Helper for mutating the [`Budget`] held in this [`Host`], either to
+    /// allocate it on contract creation or to deplete it on callbacks from
+    /// the VM or host functions.
+    #[allow(dead_code)]
+    pub(crate) fn with_budget<T, F>(&self, f: F) -> Result<T, HostError>
+    where
+        F: FnOnce(Budget) -> Result<T, HostError>,
+    {
+        f(self.0.budget.clone())
+    }
+}
+
 #[cfg(any(test, feature = "testutils"))]
 pub(crate) mod testutils {
     use std::cell::Cell;

soroban-env-host/src/host/conversion.rs

@@ -40,32 +40,6 @@ impl Host {
         self.usize_to_u32(u).map(|v| v.into())
     }
 
-    // Notes on metering: free
-    pub(crate) fn usize_from_rawval_u32_input(
-        &self,
-        name: &'static str,
-        r: Val,
-    ) -> Result<usize, HostError> {
-        self.u32_from_rawval_input(name, r).map(|u| u as usize)
-    }
-
-    // Notes on metering: free
-    pub(crate) fn u32_from_rawval_input(
-        &self,
-        name: &'static str,
-        r: Val,
-    ) -> Result<u32, HostError> {
-        match u32::try_from(r) {
-            Ok(v) => Ok(v),
-            Err(cvt) => Err(self.err(
-                ScErrorType::Value,
-                ScErrorCode::UnexpectedType,
-                "expecting U32Val",
-                &[r],
-            )),
-        }
-    }
-
     pub(crate) fn u256_from_account(&self, account_id: &AccountId) -> Result<Uint256, HostError> {
         let crate::xdr::PublicKey::PublicKeyTypeEd25519(ed25519) =
             account_id.metered_clone(self)?.0;
@@ -81,11 +55,11 @@ impl Host {
         let u: u32 = r.into();
         match u8::try_from(u) {
             Ok(v) => Ok(v),
-            Err(cvt) => Err(self.err(
+            Err(_) => Err(self.err(
                 ScErrorType::Value,
                 ScErrorCode::InvalidInput,
                 "expecting U32Val less than 256",
-                &[r.to_val()],
+                &[r.to_val(), name.try_into_val(self)?],
             )),
         }
     }
@@ -119,7 +93,7 @@ impl Host {
                 self.charge_budget(ContractCostType::HostMemCpy, Some(N as u64))?;
                 Ok(arr.into())
             }
-            Err(cvt) => Err(err!(
+            Err(_) => Err(err!(
                 self,
                 (ScErrorType::Object, ScErrorCode::UnexpectedSize),
                 "expected fixed-length bytes slice, got slice with different size",

soroban-env-host/src/host/crypto.rs

@@ -11,15 +11,6 @@ use sha3::Keccak256;
 
 impl Host {
     // Ed25519 functions
-
-    pub(crate) fn ed25519_signature_from_bytes(
-        &self,
-        name: &'static str,
-        bytes: &[u8],
-    ) -> Result<ed25519_dalek::Signature, HostError> {
-        self.fixed_length_bytes_from_slice::<ed25519_dalek::Signature, {ed25519_dalek::SIGNATURE_LENGTH}>(name, bytes)
-    }
-
     pub(crate) fn ed25519_signature_from_bytesobj_input(
         &self,
         name: &'static str,
@@ -83,6 +74,10 @@ impl Host {
 
     // ECDSA secp256k1 functions
 
+    #[cfg(any(test, feature = "testutils"))]
+    // FIXME: this operation is dead code besides the code that claibrates it;
+    // the interface we settled on doesn't involve users converting bytes to
+    // public keys. see https://github.com/stellar/rs-soroban-env/issues/1087
     pub(crate) fn secp256k1_pub_key_from_bytes(
         &self,
         bytes: &[u8],
@@ -98,15 +93,6 @@ impl Host {
         })
     }
 
-    pub(crate) fn secp256k1_pub_key_from_bytesobj_input(
-        &self,
-        k: BytesObject,
-    ) -> Result<k256::PublicKey, HostError> {
-        self.visit_obj(k, |bytes: &ScBytes| {
-            self.secp256k1_pub_key_from_bytes(bytes.as_slice())
-        })
-    }
-
     pub(crate) fn secp256k1_signature_from_bytes(
         &self,
         bytes: &[u8],