Ensure that we have the full length of component and module sections (#1492)

fitzgen · web-flow · commit 1e2ffb8f0535 · 2024-04-11T22:46:34.000Z
* Ensure that we have the full length of component and module sections Fixes the root cause of bytecodealliance/wasmtime#8322 * Extend validation fuzzer to check that payloads are in bounds * Don't check module/component section ranges; rename to `unchecked_range`; document * rustfmt
diff --git a/crates/wasm-metadata/src/lib.rs b/crates/wasm-metadata/src/lib.rs
@@ -418,8 +418,14 @@ impl Metadata {
                         }
                     }
                 }
-                ModuleSection { range, .. } => metadata.push(Metadata::empty_module(range)),
-                ComponentSection { range, .. } => metadata.push(Metadata::empty_component(range)),
+                ModuleSection {
+                    unchecked_range: range,
+                    ..
+                } => metadata.push(Metadata::empty_module(range)),
+                ComponentSection {
+                    unchecked_range: range,
+                    ..
+                } => metadata.push(Metadata::empty_component(range)),
                 End { .. } => {
                     let finished = metadata.pop().expect("non-empty metadata stack");
                     if metadata.is_empty() {
diff --git a/crates/wasmparser/src/parser.rs b/crates/wasmparser/src/parser.rs
@@ -212,7 +212,10 @@ pub enum Payload<'a> {
         parser: Parser,
         /// The range of bytes that represent the nested module in the
         /// original byte stream.
-        range: Range<usize>,
+        ///
+        /// Note that, to better support streaming parsing and validation, the
+        /// validator does *not* check that this range is in bounds.
+        unchecked_range: Range<usize>,
     },
     /// A core instance section was received and the provided parser can be
     /// used to parse the contents of the core instance section.
@@ -241,7 +244,10 @@ pub enum Payload<'a> {
         parser: Parser,
         /// The range of bytes that represent the nested component in the
         /// original byte stream.
-        range: Range<usize>,
+        ///
+        /// Note that, to better support streaming parsing and validation, the
+        /// validator does *not* check that this range is in bounds.
+        unchecked_range: Range<usize>,
     },
     /// A component instance section was received and the provided reader can be
     /// used to parse the contents of the component instance section.
@@ -679,16 +685,22 @@ impl Parser {
                             );
                         }
 
-                        let range =
-                            reader.original_position()..reader.original_position() + len as usize;
+                        let range = reader.original_position()
+                            ..reader.original_position() + usize::try_from(len).unwrap();
                         self.max_size -= u64::from(len);
                         self.offset += u64::from(len);
                         let mut parser = Parser::new(usize_to_u64(reader.original_position()));
-                        parser.max_size = len.into();
+                        parser.max_size = u64::from(len);
 
                         Ok(match id {
-                            1 => ModuleSection { parser, range },
-                            4 => ComponentSection { parser, range },
+                            1 => ModuleSection {
+                                parser,
+                                unchecked_range: range,
+                            },
+                            4 => ComponentSection {
+                                parser,
+                                unchecked_range: range,
+                            },
                             _ => unreachable!(),
                         })
                     }
@@ -1098,10 +1110,16 @@ impl Payload<'_> {
             CodeSectionStart { range, .. } => Some((CODE_SECTION, range.clone())),
             CodeSectionEntry(_) => None,
 
-            ModuleSection { range, .. } => Some((COMPONENT_MODULE_SECTION, range.clone())),
+            ModuleSection {
+                unchecked_range: range,
+                ..
+            } => Some((COMPONENT_MODULE_SECTION, range.clone())),
             InstanceSection(s) => Some((COMPONENT_CORE_INSTANCE_SECTION, s.range())),
             CoreTypeSection(s) => Some((COMPONENT_CORE_TYPE_SECTION, s.range())),
-            ComponentSection { range, .. } => Some((COMPONENT_SECTION, range.clone())),
+            ComponentSection {
+                unchecked_range: range,
+                ..
+            } => Some((COMPONENT_SECTION, range.clone())),
             ComponentInstanceSection(s) => Some((COMPONENT_INSTANCE_SECTION, s.range())),
             ComponentAliasSection(s) => Some((COMPONENT_ALIAS_SECTION, s.range())),
             ComponentTypeSection(s) => Some((COMPONENT_TYPE_SECTION, s.range())),
@@ -1164,13 +1182,19 @@ impl fmt::Debug for Payload<'_> {
             CodeSectionEntry(_) => f.debug_tuple("CodeSectionEntry").field(&"...").finish(),
 
             // Component sections
-            ModuleSection { parser: _, range } => f
+            ModuleSection {
+                parser: _,
+                unchecked_range: range,
+            } => f
                 .debug_struct("ModuleSection")
                 .field("range", range)
                 .finish(),
             InstanceSection(_) => f.debug_tuple("InstanceSection").field(&"...").finish(),
             CoreTypeSection(_) => f.debug_tuple("CoreTypeSection").field(&"...").finish(),
-            ComponentSection { parser: _, range } => f
+            ComponentSection {
+                parser: _,
+                unchecked_range: range,
+            } => f
                 .debug_struct("ComponentSection")
                 .field("range", range)
                 .finish(),
diff --git a/crates/wasmparser/src/validator.rs b/crates/wasmparser/src/validator.rs
@@ -556,13 +556,21 @@ impl Validator {
             DataSection(s) => self.data_section(s)?,
 
             // Component sections
-            ModuleSection { parser, range, .. } => {
+            ModuleSection {
+                parser,
+                unchecked_range: range,
+                ..
+            } => {
                 self.module_section(range)?;
                 return Ok(ValidPayload::Parser(parser.clone()));
             }
             InstanceSection(s) => self.instance_section(s)?,
             CoreTypeSection(s) => self.core_type_section(s)?,
-            ComponentSection { parser, range, .. } => {
+            ComponentSection {
+                parser,
+                unchecked_range: range,
+                ..
+            } => {
                 self.component_section(range)?;
                 return Ok(ValidPayload::Parser(parser.clone()));
             }
diff --git a/crates/wasmprinter/src/lib.rs b/crates/wasmprinter/src/lib.rs
@@ -267,7 +267,14 @@ impl Printer {
                 Payload::CodeSectionEntry(f) => {
                     code.push(f);
                 }
-                Payload::ModuleSection { range, .. } | Payload::ComponentSection { range, .. } => {
+                Payload::ModuleSection {
+                    unchecked_range: range,
+                    ..
+                }
+                | Payload::ComponentSection {
+                    unchecked_range: range,
+                    ..
+                } => {
                     let offset = range.end - range.start;
                     if offset > bytes.len() {
                         bail!("invalid module or component section range");
@@ -514,7 +521,7 @@ impl Printer {
 
                 Payload::ModuleSection {
                     parser: inner,
-                    range,
+                    unchecked_range: range,
                 } => {
                     Self::ensure_component(&states)?;
                     expected = Some(Encoding::Module);
@@ -529,7 +536,7 @@ impl Printer {
                 Payload::CoreTypeSection(s) => self.print_core_types(&mut states, s)?,
                 Payload::ComponentSection {
                     parser: inner,
-                    range,
+                    unchecked_range: range,
                 } => {
                     Self::ensure_component(&states)?;
                     expected = Some(Encoding::Component);
diff --git a/fuzz/src/incremental_parse.rs b/fuzz/src/incremental_parse.rs
@@ -146,9 +146,11 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
             (
                 ModuleSection {
                     parser: p,
-                    range: a,
+                    unchecked_range: a,
+                },
+                ModuleSection {
+                    unchecked_range: b, ..
                 },
-                ModuleSection { range: b, .. },
             ) => {
                 assert_eq!(a, b);
                 stack.push(parser);
@@ -158,9 +160,11 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
             (
                 ComponentSection {
                     parser: p,
-                    range: a,
+                    unchecked_range: a,
+                },
+                ComponentSection {
+                    unchecked_range: b, ..
                 },
-                ComponentSection { range: b, .. },
             ) => {
                 assert_eq!(a, b);
                 stack.push(parser);
diff --git a/fuzz/src/validate.rs b/fuzz/src/validate.rs
@@ -1,5 +1,5 @@
 use arbitrary::{Result, Unstructured};
-use wasmparser::{Validator, WasmFeatures};
+use wasmparser::{Parser, Validator, WasmFeatures};
 
 pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
     // Either use `wasm-smith` to generate a module with possibly invalid
@@ -21,14 +21,13 @@ pub fn validate_maybe_invalid_module(u: &mut Unstructured<'_>) -> Result<()> {
         config.allow_invalid_funcs = true;
         Ok(())
     })?;
-    let mut validator = crate::validator_for_config(&config);
-    drop(validator.validate_all(&wasm));
+    validate_all(crate::validator_for_config(&config), &wasm);
     Ok(())
 }
 
 pub fn validate_raw_bytes(u: &mut Unstructured<'_>) -> Result<()> {
     // Enable arbitrary combinations of features to validate the input bytes.
-    let mut validator = Validator::new_with_features(WasmFeatures {
+    let validator = Validator::new_with_features(WasmFeatures {
         reference_types: u.arbitrary()?,
         multi_value: u.arbitrary()?,
         threads: u.arbitrary()?,
@@ -55,6 +54,65 @@ pub fn validate_raw_bytes(u: &mut Unstructured<'_>) -> Result<()> {
     });
     let wasm = u.bytes(u.len())?;
     crate::log_wasm(wasm, "");
-    drop(validator.validate_all(wasm));
+    validate_all(validator, wasm);
     Ok(())
 }
+
+fn validate_all(mut validator: Validator, wasm: &[u8]) {
+    for payload in Parser::new(0).parse_all(wasm) {
+        let payload = match payload {
+            Ok(p) => p,
+            Err(_) => return,
+        };
+
+        if validator.payload(&payload).is_err() {
+            return;
+        }
+
+        // Check that the payload's range is in bounds, since the payload is
+        // supposedly valid.
+        use wasmparser::Payload::*;
+        match payload {
+            Version { range, .. } => assert!(wasm.get(range).is_some()),
+            TypeSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ImportSection(s) => assert!(wasm.get(s.range()).is_some()),
+            FunctionSection(s) => assert!(wasm.get(s.range()).is_some()),
+            TableSection(s) => assert!(wasm.get(s.range()).is_some()),
+            MemorySection(s) => assert!(wasm.get(s.range()).is_some()),
+            TagSection(s) => assert!(wasm.get(s.range()).is_some()),
+            GlobalSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ExportSection(s) => assert!(wasm.get(s.range()).is_some()),
+            StartSection { range, .. } => assert!(wasm.get(range).is_some()),
+            ElementSection(s) => assert!(wasm.get(s.range()).is_some()),
+            DataCountSection { range, .. } => assert!(wasm.get(range).is_some()),
+            DataSection(s) => assert!(wasm.get(s.range()).is_some()),
+            CodeSectionStart { range, .. } => assert!(wasm.get(range).is_some()),
+            CodeSectionEntry(body) => assert!(wasm.get(body.range()).is_some()),
+            InstanceSection(s) => assert!(wasm.get(s.range()).is_some()),
+            CoreTypeSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentInstanceSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentAliasSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentTypeSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentCanonicalSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentStartSection { range, .. } => assert!(wasm.get(range).is_some()),
+            ComponentImportSection(s) => assert!(wasm.get(s.range()).is_some()),
+            ComponentExportSection(s) => assert!(wasm.get(s.range()).is_some()),
+            CustomSection(s) => assert!(wasm.get(s.range()).is_some()),
+            UnknownSection { range, .. } => assert!(wasm.get(range).is_some()),
+
+            // In order to support streaming parsing and validation, these
+            // sections' ranges are not checked during validation, since they
+            // contain nested sections and we don't want to require all nested
+            // sections are present before we can parse/validate any of them.
+            ComponentSection {
+                unchecked_range: _, ..
+            }
+            | ModuleSection {
+                unchecked_range: _, ..
+            } => {}
+
+            // No associated range.
+            End(_) => {}
+        }
+    }
+}
diff --git a/src/bin/wasm-tools/dump.rs b/src/bin/wasm-tools/dump.rs
@@ -270,7 +270,10 @@ impl<'a> Dump<'a> {
                 }
 
                 // Component sections
-                Payload::ModuleSection { range, .. } => {
+                Payload::ModuleSection {
+                    unchecked_range: range,
+                    ..
+                } => {
                     write!(
                         self.state,
                         "[core module {}] inline size",
@@ -297,7 +300,10 @@ impl<'a> Dump<'a> {
                     me.print(end)
                 })?,
 
-                Payload::ComponentSection { range, .. } => {
+                Payload::ComponentSection {
+                    unchecked_range: range,
+                    ..
+                } => {
                     write!(
                         self.state,
                         "[component {}] inline size",
diff --git a/src/bin/wasm-tools/objdump.rs b/src/bin/wasm-tools/objdump.rs
@@ -49,13 +49,19 @@ impl Opts {
                 }
                 CodeSectionEntry(_) => {}
 
-                ModuleSection { range, .. } => {
+                ModuleSection {
+                    unchecked_range: range,
+                    ..
+                } => {
                     printer.section_raw(range, 1, "module")?;
                     printer.start(Encoding::Module)?;
                 }
                 InstanceSection(s) => printer.section(s, "core instances")?,
                 CoreTypeSection(s) => printer.section(s, "core types")?,
-                ComponentSection { range, .. } => {
+                ComponentSection {
+                    unchecked_range: range,
+                    ..
+                } => {
                     printer.section_raw(range, 1, "component")?;
                     printer.indices.push(IndexSpace::default());
                     printer.start(Encoding::Component)?;

Original file line number	Diff line number	Diff line change
`@@ -418,8 +418,14 @@ impl Metadata {`
`418`	`418`	`}`
`419`	`419`	`}`
`420`	`420`	`}`
`421`		`- ModuleSection { range, .. } => metadata.push(Metadata::empty_module(range)),`
`422`		`- ComponentSection { range, .. } => metadata.push(Metadata::empty_component(range)),`
	`421`	`+ ModuleSection {`
	`422`	`+ unchecked_range: range,`
	`423`	`+ ..`
	`424`	`+ } => metadata.push(Metadata::empty_module(range)),`
	`425`	`+ ComponentSection {`
	`426`	`+ unchecked_range: range,`
	`427`	`+ ..`
	`428`	`+ } => metadata.push(Metadata::empty_component(range)),`
`423`	`429`	`End { .. } => {`
`424`	`430`	`let finished = metadata.pop().expect("non-empty metadata stack");`
`425`	`431`	`if metadata.is_empty() {`