20.1.0 cleanup (#1314)

Cargo.toml

@@ -16,14 +16,14 @@ members = [
 exclude = ["soroban-test-wasms/wasm-workspace"]
 
 [workspace.package]
-version = "20.0.1"
+version = "20.1.0"
 
 [workspace.dependencies]
-soroban-env-common = { version = "=20.0.1", path = "soroban-env-common", default-features = false }
-soroban-env-guest = { version = "=20.0.1", path = "soroban-env-guest" }
-soroban-env-host = { version = "=20.0.1", path = "soroban-env-host" }
-soroban-env-macros = { version = "=20.0.1", path = "soroban-env-macros" }
-soroban-builtin-sdk-macros = { version = "=20.0.1", path = "soroban-builtin-sdk-macros" }
+soroban-env-common = { version = "=20.1.0", path = "soroban-env-common", default-features = false }
+soroban-env-guest = { version = "=20.1.0", path = "soroban-env-guest" }
+soroban-env-host = { version = "=20.1.0", path = "soroban-env-host" }
+soroban-env-macros = { version = "=20.1.0", path = "soroban-env-macros" }
+soroban-builtin-sdk-macros = { version = "=20.1.0", path = "soroban-builtin-sdk-macros" }
 
 # NB: When updating, also update the version in rs-soroban-env dev-dependencies
 [workspace.dependencies.stellar-xdr]

soroban-env-common/src/convert.rs

@@ -525,11 +525,11 @@ where
                 unsafe { Val::from_body_and_tag(i.lo_lo, Tag::I256Small) }
             }
             ScVal::Symbol(bytes) => {
-                let ss = match std::str::from_utf8(bytes.as_slice()) {
-                    Ok(ss) => ss,
-                    Err(_) => return Err(ConversionError),
-                };
-                SymbolSmall::try_from_str(ss)?.into()
+                // NB: Long symbols are objects and should have been
+                // handled before reaching this point.
+                SymbolSmall::try_from_bytes(bytes.as_slice())
+                    .map_err(|_| ConversionError {})?
+                    .into()
             }
 
             // These should all have been classified as ScValObjRef above, or are

soroban-env-common/src/env.rs

@@ -168,7 +168,7 @@ pub trait EnvBase: Sized + Clone {
     fn string_new_from_slice(&self, slice: &[u8]) -> Result<StringObject, Self::Error>;
 
     /// Form a new `Symbol` host object from a slice of client memory.
-    fn symbol_new_from_slice(&self, slice: &str) -> Result<SymbolObject, Self::Error>;
+    fn symbol_new_from_slice(&self, slice: &[u8]) -> Result<SymbolObject, Self::Error>;
 
     /// Form a new `Map` host object from a slice of symbol-names and a slice of values.
     /// Keys must be in sorted order.

soroban-env-common/src/symbol.rs

@@ -20,8 +20,8 @@
 //!      small maximum size for [`SymbolObject`]s, given by
 //!      [`SCSYMBOL_LIMIT`](crate::xdr::SCSYMBOL_LIMIT) (currently 32 bytes).
 //!      Having such a modest maximum size allows working with `Symbol`s
-//!      entirely in guest WASM code without a heap allocator, using only
-//!      fixed-size buffers on the WASM shadow stack. The [`SymbolStr`] type is
+//!      entirely in guest Wasm code without a heap allocator, using only
+//!      fixed-size buffers on the Wasm shadow stack. The [`SymbolStr`] type is
 //!      a convenience wrapper around such a buffer, that can be directly
 //!      created from a [`Symbol`], copying its bytes from the host if the
 //!      `Symbol` is a `SymbolObject` or unpacking its 6-bit codes if the
@@ -45,9 +45,10 @@
 //! `Val` word) with zero padding in the high-order bits rather than low-order
 //! bits. While this means that lexicographical ordering of `SymbolSmall` values
 //! does not coincide with simple integer ordering of `Val` bodies, it optimizes
-//! the space cost of `SymbolSmall` literals in WASM bytecode, where all integer
+//! the space cost of `SymbolSmall` literals in Wasm bytecode, where all integer
 //! literals are encoded as variable-length little-endian values, using ULEB128.
 
+use crate::xdr::SCSYMBOL_LIMIT;
 use crate::{
     declare_tag_based_small_and_object_wrappers, val::ValConvert, Compare, ConversionError, Env,
     Tag, TryFromVal, Val,
@@ -63,8 +64,8 @@ pub enum SymbolError {
     /// than 9 characters.
     TooLong(usize),
     /// Returned when attempting to form a [SymbolObject] or [SymbolSmall] from
-    /// a string with characters outside the range `[a-zA-Z0-9_]`.
-    BadChar(char),
+    /// a byte string with characters outside the range `[a-zA-Z0-9_]`.
+    BadChar(u8),
     /// Malformed small symbol (upper two bits were set).
     MalformedSmall,
 }
@@ -103,29 +104,28 @@ sa::const_assert!(CODE_MASK == 0x3f);
 sa::const_assert!(CODE_BITS * MAX_SMALL_CHARS + 2 == BODY_BITS);
 sa::const_assert!(SMALL_MASK == 0x003f_ffff_ffff_ffff);
 
-impl<E: Env> TryFromVal<E, &str> for Symbol {
+impl<E: Env> TryFromVal<E, &[u8]> for Symbol {
     type Error = crate::Error;
 
-    fn try_from_val(env: &E, v: &&str) -> Result<Self, Self::Error> {
-        if let Ok(ss) = SymbolSmall::try_from_str(v) {
-            Ok(Self(ss.0))
+    fn try_from_val(env: &E, v: &&[u8]) -> Result<Self, Self::Error> {
+        // Optimization note: this should only ever call one conversion
+        // function based on the input slice length, currently slices
+        // with invalid characters get re-validated.
+        if let Ok(s) = SymbolSmall::try_from_bytes(v) {
+            Ok(s.into())
         } else {
-            let sobj = env.symbol_new_from_slice(v).map_err(Into::into)?;
-            Ok(Self(sobj.0))
+            env.symbol_new_from_slice(v)
+                .map(Into::into)
+                .map_err(Into::into)
         }
     }
 }
 
-impl<E: Env> TryFromVal<E, &[u8]> for Symbol {
+impl<E: Env> TryFromVal<E, &str> for Symbol {
     type Error = crate::Error;
 
-    fn try_from_val(env: &E, v: &&[u8]) -> Result<Self, Self::Error> {
-        // We don't know this byte-slice is actually utf-8 ...
-        let s: &str = unsafe { core::str::from_utf8_unchecked(v) };
-        // ... but this next conversion step will check that its
-        // _bytes_ are in the symbol-char range, which is a subset
-        // of utf-8, so we're only lying harmlessly.
-        Symbol::try_from_val(env, &s)
+    fn try_from_val(env: &E, v: &&str) -> Result<Self, Self::Error> {
+        Symbol::try_from_val(env, &v.as_bytes())
     }
 }
 
@@ -166,20 +166,15 @@ impl SymbolSmall {
 }
 
 impl Symbol {
-    #[doc(hidden)]
-    pub const unsafe fn from_small_body(body: u64) -> Self {
-        // This is a helper that should only be used in macros where it's
-        // necessary to generate a Symbol constant with no possibility of error:
-        // it just forces the bits that might be set wrong (54 and 55) to zero.
-        let body = body & SMALL_MASK;
-        Symbol(SymbolSmall::from_body(body).0)
+    pub const fn try_from_small_bytes(b: &[u8]) -> Result<Self, SymbolError> {
+        match SymbolSmall::try_from_bytes(b) {
+            Ok(sym) => Ok(Symbol(sym.0)),
+            Err(e) => Err(e),
+        }
     }
 
     pub const fn try_from_small_str(s: &str) -> Result<Self, SymbolError> {
-        match SymbolSmall::try_from_str(s) {
-            Ok(ss) => Ok(Symbol(ss.0)),
-            Err(e) => Err(e),
-        }
+        Self::try_from_small_bytes(s.as_bytes())
     }
 
     // This should not be generally available as it can easily panic.
@@ -205,7 +200,6 @@ impl TryFrom<&[u8]> for SymbolSmall {
 
 #[cfg(feature = "std")]
 use crate::xdr::StringM;
-use crate::xdr::SCSYMBOL_LIMIT;
 #[cfg(feature = "std")]
 impl<const N: u32> TryFrom<StringM<N>> for SymbolSmall {
     type Error = SymbolError;
@@ -225,39 +219,38 @@ impl<const N: u32> TryFrom<&StringM<N>> for SymbolSmall {
 
 impl SymbolSmall {
     #[doc(hidden)]
-    pub const fn validate_char(ch: char) -> Result<(), SymbolError> {
-        match SymbolSmall::encode_char(ch) {
+    pub const fn validate_byte(b: u8) -> Result<(), SymbolError> {
+        match Self::encode_byte(b) {
             Ok(_) => Ok(()),
             Err(e) => Err(e),
         }
     }
 
-    const fn encode_char(ch: char) -> Result<u64, SymbolError> {
-        let v = match ch {
-            '_' => 1,
-            '0'..='9' => 2 + ((ch as u64) - ('0' as u64)),
-            'A'..='Z' => 12 + ((ch as u64) - ('A' as u64)),
-            'a'..='z' => 38 + ((ch as u64) - ('a' as u64)),
-            _ => return Err(SymbolError::BadChar(ch)),
+    const fn encode_byte(b: u8) -> Result<u8, SymbolError> {
+        let v = match b {
+            b'_' => 1,
+            b'0'..=b'9' => 2 + (b - b'0'),
+            b'A'..=b'Z' => 12 + (b - b'A'),
+            b'a'..=b'z' => 38 + (b - b'a'),
+            _ => return Err(SymbolError::BadChar(b)),
         };
         Ok(v)
     }
 
-    pub const fn try_from_bytes(b: &[u8]) -> Result<SymbolSmall, SymbolError> {
+    pub const fn try_from_bytes(bytes: &[u8]) -> Result<SymbolSmall, SymbolError> {
+        if bytes.len() > MAX_SMALL_CHARS {
+            return Err(SymbolError::TooLong(bytes.len()));
+        }
         let mut n = 0;
         let mut accum: u64 = 0;
-        while n < b.len() {
-            let ch = b[n] as char;
-            if n >= MAX_SMALL_CHARS {
-                return Err(SymbolError::TooLong(b.len()));
-            }
-            n += 1;
-            accum <<= CODE_BITS;
-            let v = match SymbolSmall::encode_char(ch) {
+        while n < bytes.len() {
+            let v = match Self::encode_byte(bytes[n]) {
                 Ok(v) => v,
                 Err(e) => return Err(e),
             };
-            accum |= v;
+            accum <<= CODE_BITS;
+            accum |= v as u64;
+            n += 1;
         }
         Ok(unsafe { Self::from_body(accum) })
     }
@@ -328,6 +321,9 @@ impl AsRef<[u8]> for SymbolStr {
     }
 }
 
+// This conversion relies on `SymbolStr` representing a well-formed `Symbol`,
+// which in turn relies on `EnvBase` implementation to only produce valid
+// `Symbol`s.
 impl AsRef<str> for SymbolStr {
     fn as_ref(&self) -> &str {
         let s: &[u8] = self.as_ref();
@@ -470,7 +466,7 @@ impl TryFrom<ScVal> for SymbolSmall {
 impl TryFrom<&ScVal> for SymbolSmall {
     type Error = crate::Error;
     fn try_from(v: &ScVal) -> Result<Self, Self::Error> {
-        if let ScVal::Symbol(crate::xdr::ScSymbol(vec)) = v {
+        if let ScVal::Symbol(ScSymbol(vec)) = v {
             vec.try_into().map_err(Into::into)
         } else {
             Err(ConversionError.into())

soroban-env-common/src/val.rs

@@ -9,7 +9,7 @@ use crate::{
 };
 
 use super::{Env, Error, TryFromVal};
-use core::{cmp::Ordering, convert::Infallible, fmt::Debug};
+use core::{cmp::Ordering, convert::Infallible, fmt::Debug, str};
 
 extern crate static_assertions as sa;
 
@@ -609,17 +609,34 @@ impl Val {
         }
     }
 
-    pub fn can_represent_scval(scv: &crate::xdr::ScVal) -> bool {
+    /// *Non-recursively* checks whether `ScVal` can be represented as `Val`.
+    /// Since conversions from `ScVal` are recursive themselves, this should
+    /// be called at every recursion level during conversion.
+    pub fn can_represent_scval(scv: &ScVal) -> bool {
+        match scv {
+            // Map/vec can't be validated just based on the discriminant,
+            // as their contents can't be `None`.
+            ScVal::Vec(None) => return false,
+            ScVal::Map(None) => return false,
+            _ => Self::can_represent_scval_type(scv.discriminant()),
+        }
+    }
+
+    /// *Recursively* checks whether `ScVal` can be represented as `Val`.
+    /// This should only be used once per top-level `ScVal`.
+    pub fn can_represent_scval_recursive(scv: &ScVal) -> bool {
         match scv {
             // Handle recursive types first
             ScVal::Vec(None) => return false,
             ScVal::Map(None) => return false,
-            ScVal::Vec(Some(v)) => return v.0.iter().all(|x| Val::can_represent_scval(x)),
+            ScVal::Vec(Some(v)) => {
+                return v.0.iter().all(|x| Val::can_represent_scval_recursive(x))
+            }
             ScVal::Map(Some(m)) => {
-                return m
-                    .0
-                    .iter()
-                    .all(|e| Val::can_represent_scval(&e.key) && Val::can_represent_scval(&e.val))
+                return m.0.iter().all(|e| {
+                    Val::can_represent_scval_recursive(&e.key)
+                        && Val::can_represent_scval_recursive(&e.val)
+                })
             }
             _ => Self::can_represent_scval_type(scv.discriminant()),
         }
@@ -820,8 +837,12 @@ impl Debug for Val {
             Tag::SymbolSmall => {
                 let ss: SymbolStr =
                     unsafe { <SymbolSmall as ValConvert>::unchecked_from_val(*self) }.into();
+                // Even though this may be called for an arbitrary, not necessarily well-formed
+                // `Val`, this is still safe thanks to `SymbolSmall` iteration implementation that
+                // only returns valid symbol characters or `\0` even for invalid bit
+                // representations.
                 let s: &str = ss.as_ref();
-                write!(f, "Symbol({s})")
+                write!(f, "Symbol({})", s)
             }
 
             Tag::U64Object => fmt_obj("U64", self, f),

soroban-env-guest/src/guest.rs

@@ -115,7 +115,7 @@ impl EnvBase for Guest {
         self.string_new_from_linear_memory(lm_pos, len)
     }
 
-    fn symbol_new_from_slice(&self, slice: &str) -> Result<SymbolObject, Self::Error> {
+    fn symbol_new_from_slice(&self, slice: &[u8]) -> Result<SymbolObject, Self::Error> {
         sa::assert_eq_size!(u32, *const u8);
         sa::assert_eq_size!(u32, usize);
         let lm_pos: U32Val = Val::from_u32(slice.as_ptr() as u32);