Repository: bytecodealliance/wasm-tools (rust)

Issue:
Flawed logic in `fn check_non_deterministic_enabled`
The logic in this function looks like it is flawed. Or I misunderstand the intent.

https://github.com/bytecodealliance/wasm-tools/blob/57aad847b9e97f577e157df60b19f1e0710cd70d/crates/wasmparser/src/validator/operators.rs#L562-L568

I think it should look like this:
```rust
fn check_non_deterministic_enabled(&self) -> Result<()> {
    if cfg!(feature = "deterministic") || self.features.deterministic_only {
        bail!(self.offset, "deterministic_only support is not enabled");
    }
    Ok(())
}
```

 I think it is quite confusing and maybe even dangerous if `deterministic_only`  silently does nothing if the feature is not set.


Pull request description:
Remove `deterministic` crate feature from `wasmparser
This is a very old feature of the `wasmparser` crate which I don't think fits well within the current design of the crate. This is replaced with a field in `WasmFeatures` that's always checked and always available to work like other proposals. Additionally the `deterministic_only` field in `WasmFeatures` has been inverted and renamed as `floats` to work like other fields where enabling means allowing more behavior. This also encompasses what the flag actually gates right now, which is just floats.

Closes #840

Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 75c8dcb3..95d4508a 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -42,7 +42,6 @@ jobs:
       run: rustup update ${{ matrix.rust }} --no-self-update && rustup default ${{ matrix.rust }}
       shell: bash
     - run: cargo test --all
-    - run: cargo test --manifest-path crates/wasmparser/Cargo.toml --features deterministic
     - run: cargo test -p wasmparser --benches
     - run: cargo build --manifest-path crates/wast/Cargo.toml --no-default-features
     - run: cargo build --manifest-path crates/wast/Cargo.toml --no-default-features --features wasm-module
diff --git a/crates/wasm-shrink/src/lib.rs b/crates/wasm-shrink/src/lib.rs
index 09a8b0b2..a64a3bea 100755
--- a/crates/wasm-shrink/src/lib.rs
+++ b/crates/wasm-shrink/src/lib.rs
@@ -233,9 +233,7 @@ impl ShrinkRun {
             saturating_float_to_int: true,
             sign_extension: true,
             component_model: false,
-
-            // We'll never enable this here.
-            deterministic_only: false,
+            floats: true,
         });
 
         validator.validate_all(wasm)?;
diff --git a/crates/wasmparser/Cargo.toml b/crates/wasmparser/Cargo.toml
index d9674c07..aaed06ce 100644
--- a/crates/wasmparser/Cargo.toml
+++ b/crates/wasmparser/Cargo.toml
@@ -28,9 +28,3 @@ once_cell = "1.13.0"
 [[bench]]
 name = "benchmark"
 harness = false
-
-[features]
-# The "deterministic" feature supports only Wasm code with "deterministic" execution
-# across any hardware. This feature is very critical for many Blockchain infrastructures
-# that rely on deterministic executions of smart contracts across different hardwares.
-deterministic = []
diff --git a/crates/wasmparser/benches/benchmark.rs b/crates/wasmparser/benches/benchmark.rs
index bddd0871..17fa72e3 100644
--- a/crates/wasmparser/benches/benchmark.rs
+++ b/crates/wasmparser/benches/benchmark.rs
@@ -247,7 +247,7 @@ fn define_benchmarks(c: &mut Criterion) {
             multi_memory: true,
             memory64: true,
             extended_const: true,
-            deterministic_only: false,
+            floats: true,
             mutable_global: true,
             saturating_float_to_int: true,
             sign_extension: true,
diff --git a/crates/wasmparser/src/validator.rs b/crates/wasmparser/src/validator.rs
index 5e1b2de9..d5b9bbdd 100644
--- a/crates/wasmparser/src/validator.rs
+++ b/crates/wasmparser/src/validator.rs
@@ -211,7 +211,7 @@ pub struct WasmFeatures {
     pub multi_value: bool,
     /// The WebAssembly bulk memory operations proposal (enabled by default)
     pub bulk_memory: bool,
-    /// The WebAssembly SIMD proposal
+    /// The WebAssembly SIMD proposal (enabled by default)
     pub simd: bool,
     /// The WebAssembly Relaxed SIMD proposal
     pub relaxed_simd: bool,
@@ -219,8 +219,18 @@ pub struct WasmFeatures {
     pub threads: bool,
     /// The WebAssembly tail-call proposal
     pub tail_call: bool,
-    /// Whether or not only deterministic instructions are allowed
-    pub deterministic_only: bool,
+    /// Whether or not floating-point instructions are enabled.
+    ///
+    /// This is enabled by default can be used to disallow floating-point
+    /// operators. Note that disabling this does not disable the `f32` and
+    /// `f64` wasm types, only the operators that work on them.
+    ///
+    /// This does not correspond to a WebAssembly proposal but is instead
+    /// intended for embeddings which have stricter-than-usual requirements
+    /// about execution. Floats in WebAssembly can have different NaN patterns
+    /// across hosts which can lead to host-dependent execution which some
+    /// runtimes may not desire.
+    pub floats: bool,
     /// The WebAssembly multi memory proposal
     pub multi_memory: bool,
     /// The WebAssembly exception handling proposal
@@ -267,7 +277,6 @@ impl Default for WasmFeatures {
             memory64: false,
             extended_const: false,
             component_model: false,
-            deterministic_only: cfg!(feature = "deterministic"),
 
             // on-by-default features
             mutable_global: true,
@@ -277,6 +286,7 @@ impl Default for WasmFeatures {
             multi_value: true,
             reference_types: true,
             simd: true,
+            floats: true,
         }
     }
 }
diff --git a/crates/wasmparser/src/validator/operators.rs b/crates/wasmparser/src/validator/operators.rs
index 3acb07cd..6e32b184 100644
--- a/crates/wasmparser/src/validator/operators.rs
+++ b/crates/wasmparser/src/validator/operators.rs
@@ -559,10 +559,9 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
         Ok(index_ty)
     }
 
-    #[cfg_attr(not(feature = "deterministic"), inline(always))]
-    fn check_non_deterministic_enabled(&self) -> Result<()> {
-        if cfg!(feature = "deterministic") && !self.features.deterministic_only {
-            bail!(self.offset, "deterministic_only support is not enabled");
+    fn check_floats_enabled(&self) -> Result<()> {
+        if !self.features.floats {
+            bail!(self.offset, "floating-point instruction disallowed");
         }
         Ok(())
     }
@@ -678,7 +677,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
     /// Checks the validity of a common float comparison operator.
     fn check_fcmp_op(&mut self, ty: ValType) -> Result<()> {
         debug_assert!(matches!(ty, ValType::F32 | ValType::F64));
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_cmp_op(ty)
     }
 
@@ -692,7 +691,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
     /// Checks the validity of a common unary float operator.
     fn check_funary_op(&mut self, ty: ValType) -> Result<()> {
         debug_assert!(matches!(ty, ValType::F32 | ValType::F64));
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_unary_op(ty)
     }
 
@@ -706,7 +705,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
     /// Checks the validity of a common conversion operator.
     fn check_fconversion_op(&mut self, into: ValType, from: ValType) -> Result<()> {
         debug_assert!(matches!(into, ValType::F32 | ValType::F64));
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_conversion_op(into, from)
     }
 
@@ -721,7 +720,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
     /// Checks the validity of a common binary float operator.
     fn check_fbinary_op(&mut self, ty: ValType) -> Result<()> {
         debug_assert!(matches!(ty, ValType::F32 | ValType::F64));
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_binary_op(ty)
     }
 
@@ -777,7 +776,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
 
     /// Checks a [`V128`] binary float operator.
     fn check_v128_fbinary_op(&mut self) -> Result<()> {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_v128_binary_op()
     }
 
@@ -798,7 +797,7 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R
 
     /// Checks a [`V128`] binary operator.
     fn check_v128_funary_op(&mut self) -> Result<()> {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_v128_unary_op()
     }
 
@@ -1277,14 +1276,14 @@ where
         Ok(())
     }
     fn visit_f32_load(&mut self, memarg: MemArg) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         let ty = self.check_memarg(memarg)?;
         self.pop_operand(Some(ty))?;
         self.push_operand(ValType::F32)?;
         Ok(())
     }
     fn visit_f64_load(&mut self, memarg: MemArg) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         let ty = self.check_memarg(memarg)?;
         self.pop_operand(Some(ty))?;
         self.push_operand(ValType::F64)?;
@@ -1348,14 +1347,14 @@ where
         Ok(())
     }
     fn visit_f32_store(&mut self, memarg: MemArg) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         let ty = self.check_memarg(memarg)?;
         self.pop_operand(Some(ValType::F32))?;
         self.pop_operand(Some(ty))?;
         Ok(())
     }
     fn visit_f64_store(&mut self, memarg: MemArg) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         let ty = self.check_memarg(memarg)?;
         self.pop_operand(Some(ValType::F64))?;
         self.pop_operand(Some(ty))?;
@@ -1417,12 +1416,12 @@ where
         Ok(())
     }
     fn visit_f32_const(&mut self, _value: Ieee32) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.push_operand(ValType::F32)?;
         Ok(())
     }
     fn visit_f64_const(&mut self, _value: Ieee64) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.push_operand(ValType::F64)?;
         Ok(())
     }
@@ -2117,11 +2116,11 @@ where
         self.check_v128_splat(ValType::I64)
     }
     fn visit_f32x4_splat(&mut self) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_v128_splat(ValType::F32)
     }
     fn visit_f64x2_splat(&mut self) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_v128_splat(ValType::F64)
     }
     fn visit_i8x16_extract_lane_s(&mut self, lane: u8) -> Self::Output {
@@ -2183,14 +2182,14 @@ where
         Ok(())
     }
     fn visit_f32x4_extract_lane(&mut self, lane: u8) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_simd_lane_index(lane, 4)?;
         self.pop_operand(Some(ValType::V128))?;
         self.push_operand(ValType::F32)?;
         Ok(())
     }
     fn visit_f32x4_replace_lane(&mut self, lane: u8) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_simd_lane_index(lane, 4)?;
         self.pop_operand(Some(ValType::F32))?;
         self.pop_operand(Some(ValType::V128))?;
@@ -2198,14 +2197,14 @@ where
         Ok(())
     }
     fn visit_f64x2_extract_lane(&mut self, lane: u8) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_simd_lane_index(lane, 2)?;
         self.pop_operand(Some(ValType::V128))?;
         self.push_operand(ValType::F64)?;
         Ok(())
     }
     fn visit_f64x2_replace_lane(&mut self, lane: u8) -> Self::Output {
-        self.check_non_deterministic_enabled()?;
+        self.check_floats_enabled()?;
         self.check_simd_lane_index(lane, 2)?;
         self.pop_operand(Some(ValType::F64))?;
         self.pop_operand(Some(ValType::V128))?;
diff --git a/fuzz/fuzz_targets/no-traps.rs b/fuzz/fuzz_targets/no-traps.rs
index 8c014454..e0f956c1 100644
--- a/fuzz/fuzz_targets/no-traps.rs
+++ b/fuzz/fuzz_targets/no-traps.rs
@@ -79,6 +79,11 @@ fuzz_target!(|data: &[u8]| {
                 return;
             }
 
+            // Allow out of fuel on module instantiation
+            if let Some(wasmtime::Trap::OutOfFuel) = err.downcast_ref::<wasmtime::Trap>() {
+                return;
+            }
+
             let s = err.to_string();
             // Allow "nominal" traps such as running out of fuel and the
             // module trying to allocate more resources than we'd like to
diff --git a/fuzz/fuzz_targets/validate.rs b/fuzz/fuzz_targets/validate.rs
index 3eaebc84..9fb059ee 100644
--- a/fuzz/fuzz_targets/validate.rs
+++ b/fuzz/fuzz_targets/validate.rs
@@ -27,7 +27,7 @@ fuzz_target!(|data: &[u8]| {
         component_model: (byte1 & 0b0001_0000) != 0,
         tail_call: (byte1 & 0b0010_0000) != 0,
         bulk_memory: (byte1 & 0b0100_0000) != 0,
-        deterministic_only: (byte1 & 0b1000_0000) != 0,
+        floats: (byte1 & 0b1000_0000) != 0,
         multi_memory: (byte2 & 0b0000_0001) != 0,
         memory64: (byte2 & 0b0000_0010) != 0,
         exceptions: (byte2 & 0b0000_0100) != 0,
diff --git a/src/bin/wasm-tools/validate.rs b/src/bin/wasm-tools/validate.rs
index 37ddce34..852c7bff 100644
--- a/src/bin/wasm-tools/validate.rs
+++ b/src/bin/wasm-tools/validate.rs
@@ -99,7 +99,7 @@ fn parse_features(arg: &str) -> Result<WasmFeatures> {
         ("exception-handling", |f| &mut f.exceptions),
         ("memory64", |f| &mut f.memory64),
         ("extended-const", |f| &mut f.extended_const),
-        ("deterministic", |f| &mut f.deterministic_only),
+        ("floats", |f| &mut f.floats),
         ("saturating-float-to-int", |f| {
             &mut f.saturating_float_to_int
         }),


Test patch:
diff --git a/crates/wasm-smith/tests/core.rs b/crates/wasm-smith/tests/core.rs
index e5146cc4..5279e24c 100644
--- a/crates/wasm-smith/tests/core.rs
+++ b/crates/wasm-smith/tests/core.rs
@@ -295,7 +295,7 @@ fn parser_features_from_config(config: &impl Config) -> WasmFeatures {
 
         threads: false,
         tail_call: false,
-        deterministic_only: false,
+        floats: true,
         extended_const: false,
         component_model: false,
     }
diff --git a/tests/roundtrip.rs b/tests/roundtrip.rs
index f7245760..046c9671 100644
--- a/tests/roundtrip.rs
+++ b/tests/roundtrip.rs
@@ -455,7 +455,7 @@ impl TestState {
             bulk_memory: true,
             tail_call: true,
             component_model: false,
-            deterministic_only: false,
+            floats: true,
             multi_value: true,
             multi_memory: true,
             memory64: true,