Deduplicate how many times type-related opcodes are listed (#1822)

* Deduplicate how many times type-related opcodes are listed

This commit refactors the parsing of core types in wasmparser to
deduplicate the number of times that opcodes for particular types are
listed. Previously each layer of parsing types would check for all valid
prefixes and then delegate to a "lower" parser if applicable. This meant
though that a type's opcode was listed at each layer it could be parsed,
resulting in duplication.

Instead now types only parse what is exclusive to that layer and then a
lower layer parses everything else. The only gotcha with this is to
preserve reasonable error messages (lest every error become "invalid
abstract heap type"). For this a new `BinaryReaderErrorKind` enum
(internal) was added which is used to track this and the message is
updated as parsing fails and errors progress upwards.

* Refactor `BinaryReaderError` some more

Author: alexcrichton

crates/wasmparser/src/binary_reader.rs

@@ -34,10 +34,17 @@ pub struct BinaryReaderError {
 #[derive(Debug, Clone)]
 pub(crate) struct BinaryReaderErrorInner {
     pub(crate) message: String,
+    pub(crate) kind: BinaryReaderErrorKind,
     pub(crate) offset: usize,
     pub(crate) needed_hint: Option<usize>,
 }
 
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum BinaryReaderErrorKind {
+    Custom,
+    Invalid,
+}
+
 /// The result for `BinaryReader` operations.
 pub type Result<T, E = BinaryReaderError> = core::result::Result<T, E>;
 
@@ -56,31 +63,41 @@ impl fmt::Display for BinaryReaderError {
 
 impl BinaryReaderError {
     #[cold]
-    pub(crate) fn new(message: impl Into<String>, offset: usize) -> Self {
-        let message = message.into();
+    pub(crate) fn _new(kind: BinaryReaderErrorKind, message: String, offset: usize) -> Self {
         BinaryReaderError {
             inner: Box::new(BinaryReaderErrorInner {
+                kind,
                 message,
                 offset,
                 needed_hint: None,
             }),
         }
     }
 
+    #[cold]
+    pub(crate) fn new(message: impl Into<String>, offset: usize) -> Self {
+        Self::_new(BinaryReaderErrorKind::Custom, message.into(), offset)
+    }
+
+    #[cold]
+    pub(crate) fn invalid(msg: &'static str, offset: usize) -> Self {
+        Self::_new(BinaryReaderErrorKind::Invalid, msg.into(), offset)
+    }
+
     #[cold]
     pub(crate) fn fmt(args: fmt::Arguments<'_>, offset: usize) -> Self {
         BinaryReaderError::new(args.to_string(), offset)
     }
 
     #[cold]
     pub(crate) fn eof(offset: usize, needed_hint: usize) -> Self {
-        BinaryReaderError {
-            inner: Box::new(BinaryReaderErrorInner {
-                message: "unexpected end-of-file".to_string(),
-                offset,
-                needed_hint: Some(needed_hint),
-            }),
-        }
+        let mut err = BinaryReaderError::new("unexpected end-of-file", offset);
+        err.inner.needed_hint = Some(needed_hint);
+        err
+    }
+
+    pub(crate) fn kind(&mut self) -> BinaryReaderErrorKind {
+        self.inner.kind
     }
 
     /// Get this error's message.
@@ -94,9 +111,12 @@ impl BinaryReaderError {
     }
 
     #[cfg(feature = "validate")]
-    pub(crate) fn add_context(&mut self, mut context: String) {
-        context.push_str("\n");
-        self.inner.message.insert_str(0, &context);
+    pub(crate) fn add_context(&mut self, context: String) {
+        self.inner.message = format!("{context}\n{}", self.inner.message);
+    }
+
+    pub(crate) fn set_message(&mut self, message: &str) {
+        self.inner.message = message.to_string();
     }
 }
 

crates/wasmparser/src/readers/core/types.rs

@@ -13,6 +13,7 @@
  * limitations under the License.
  */
 
+use crate::binary_reader::BinaryReaderErrorKind;
 use crate::limits::{
     MAX_WASM_FUNCTION_PARAMS, MAX_WASM_FUNCTION_RETURNS, MAX_WASM_STRUCT_FIELDS,
     MAX_WASM_SUPERTYPES, MAX_WASM_TYPES,
@@ -1709,79 +1710,65 @@ impl<'a> FromReader<'a> for ValType {
                 reader.read_u8()?;
                 Ok(ValType::V128)
             }
-            0x70 | 0x6F | 0x65 | 0x64 | 0x63 | 0x6E | 0x71 | 0x72 | 0x73 | 0x74 | 0x75 | 0x6D
-            | 0x6B | 0x6A | 0x6C | 0x69 | 0x68 => Ok(ValType::Ref(reader.read()?)),
-            _ => bail!(reader.original_position(), "invalid value type"),
+            _ => {
+                // Reclassify errors as invalid value types here because
+                // that's the "root" of what was being parsed rather than
+                // reference types.
+                let refty = reader.read().map_err(|mut e| {
+                    if let BinaryReaderErrorKind::Invalid = e.kind() {
+                        e.set_message("invalid value type");
+                    }
+                    e
+                })?;
+                Ok(ValType::Ref(refty))
+            }
         }
     }
 }
 
 impl<'a> FromReader<'a> for RefType {
     fn from_reader(reader: &mut BinaryReader<'a>) -> Result<Self> {
-        let absheapty = |byte, pos| match byte {
-            0x70 => Ok(RefType::FUNC.nullable()),
-            0x6F => Ok(RefType::EXTERN.nullable()),
-            0x6E => Ok(RefType::ANY.nullable()),
-            0x71 => Ok(RefType::NONE.nullable()),
-            0x72 => Ok(RefType::NOEXTERN.nullable()),
-            0x73 => Ok(RefType::NOFUNC.nullable()),
-            0x6D => Ok(RefType::EQ.nullable()),
-            0x6B => Ok(RefType::STRUCT.nullable()),
-            0x6A => Ok(RefType::ARRAY.nullable()),
-            0x6C => Ok(RefType::I31.nullable()),
-            0x69 => Ok(RefType::EXN.nullable()),
-            0x74 => Ok(RefType::NOEXN.nullable()),
-            0x68 => Ok(RefType::CONT.nullable()),
-            0x75 => Ok(RefType::NOCONT.nullable()),
-            _ => bail!(pos, "invalid abstract heap type"),
-        };
-
         // NB: See `FromReader<'a> for ValType` for a table of how this
         // interacts with other value encodings.
-        match reader.read()? {
-            byte @ (0x70 | 0x6F | 0x6E | 0x71 | 0x72 | 0x73 | 0x6D | 0x6B | 0x6A | 0x6C | 0x69
-            | 0x74) => {
-                let pos = reader.original_position();
-                absheapty(byte, pos)
-            }
-            0x65 => {
-                let byte = reader.read()?;
-                let pos = reader.original_position();
-                Ok(absheapty(byte, pos)?.shared().expect("must be abstract"))
-            }
-            byte @ (0x63 | 0x64) => {
-                let nullable = byte == 0x63;
-                let pos = reader.original_position();
+        let pos = reader.original_position();
+        match reader.peek()? {
+            0x63 | 0x64 => {
+                let nullable = reader.read_u8()? == 0x63;
                 RefType::new(nullable, reader.read()?)
                     .ok_or_else(|| crate::BinaryReaderError::new("type index too large", pos))
             }
-            _ => bail!(reader.original_position(), "malformed reference type"),
+            _ => {
+                // Reclassify errors as invalid reference types here because
+                // that's the "root" of what was being parsed rather than
+                // heap types.
+                let hty = reader.read().map_err(|mut e| {
+                    if let BinaryReaderErrorKind::Invalid = e.kind() {
+                        e.set_message("malformed reference type");
+                    }
+                    e
+                })?;
+                RefType::new(true, hty)
+                    .ok_or_else(|| crate::BinaryReaderError::new("type index too large", pos))
+            }
         }
     }
 }
 
 impl<'a> FromReader<'a> for HeapType {
     fn from_reader(reader: &mut BinaryReader<'a>) -> Result<Self> {
-        // NB: See `FromReader<'a> for ValType` for a table of how this
-        // interacts with other value encodings.
-        match reader.peek()? {
-            0x65 => {
-                reader.read_u8()?;
-                let ty = reader.read()?;
-                Ok(HeapType::Abstract { shared: true, ty })
-            }
-            0x70 | 0x6F | 0x6E | 0x71 | 0x72 | 0x73 | 0x6D | 0x6B | 0x6A | 0x6C | 0x68 | 0x69
-            | 0x74 | 0x75 => {
-                let ty = reader.read()?;
-                Ok(HeapType::Abstract { shared: false, ty })
-            }
-            _ => {
-                let idx = match u32::try_from(reader.read_var_s33()?) {
-                    Ok(idx) => idx,
-                    Err(_) => {
-                        bail!(reader.original_position(), "invalid indexed ref heap type");
-                    }
-                };
+        // Unconditionally read a `s33` value. If it's positive then that means
+        // it fits in a `u32` meaning that this is a valid concrete type index.
+        // If it's negative, however, then it must be an abstract heap type due
+        // to how non-concrete types are encoded (see `ValType` comments). In
+        // that situation "rewind" the reader and go back to the original bytes
+        // to parse them as an abstract heap type.
+        let mut clone = reader.clone();
+        let s33 = clone.read_var_s33()?;
+        match u32::try_from(s33) {
+            Ok(idx) => {
+                // Be sure to update `reader` with the state after the s33 was
+                // read.
+                *reader = clone;
                 let idx = PackedIndex::from_module_index(idx).ok_or_else(|| {
                     BinaryReaderError::new(
                         "type index greater than implementation limits",
@@ -1790,6 +1777,25 @@ impl<'a> FromReader<'a> for HeapType {
                 })?;
                 Ok(HeapType::Concrete(idx.unpack()))
             }
+            Err(_) => match reader.peek()? {
+                0x65 => {
+                    reader.read_u8()?;
+                    let ty = reader.read()?;
+                    Ok(HeapType::Abstract { shared: true, ty })
+                }
+                _ => {
+                    // Reclassify errors as "invalid heap type" here because
+                    // that's the "root" of what was being parsed rather than
+                    // abstract heap types.
+                    let ty = reader.read().map_err(|mut e| {
+                        if let BinaryReaderErrorKind::Invalid = e.kind() {
+                            e.set_message("invalid heap type");
+                        }
+                        e
+                    })?;
+                    Ok(HeapType::Abstract { shared: false, ty })
+                }
+            },
         }
     }
 }
@@ -1813,7 +1819,10 @@ impl<'a> FromReader<'a> for AbstractHeapType {
             0x68 => Ok(Cont),
             0x75 => Ok(NoCont),
             _ => {
-                bail!(reader.original_position(), "invalid abstract heap type");
+                return Err(BinaryReaderError::invalid(
+                    "invalid abstract heap type",
+                    reader.original_position() - 1,
+                ))
             }
         }
     }