Fix validation of atomic mutation of shared globals (#2473)

This commit updates validation of atomic-related mutation instructions,
only applicable with the shared-everything-threads proposal, to require
that the global in question is flagged as mutable. This seems like an
oversight in the current implementation, so mutability is required for
now.

Author: alexcrichton

crates/wasmparser/src/validator/operators.rs

@@ -1559,7 +1559,14 @@ where
     /// Common helper for checking the types of globals accessed with atomic RMW
     /// instructions, which only allow `i32` and `i64`.
     fn check_atomic_global_rmw_ty(&self, global_index: u32) -> Result<ValType> {
-        let ty = self.global_type_at(global_index)?.content_type;
+        let global = self.global_type_at(global_index)?;
+        if !global.mutable {
+            bail!(
+                self.offset,
+                "global is immutable: cannot modify it with `global.atomic.rmw.*`"
+            );
+        }
+        let ty = global.content_type;
         if !(ty == ValType::I32 || ty == ValType::I64) {
             bail!(
                 self.offset,
@@ -2452,7 +2459,14 @@ where
         _ordering: crate::Ordering,
         global_index: u32,
     ) -> Self::Output {
-        let ty = self.global_type_at(global_index)?.content_type;
+        let global = self.global_type_at(global_index)?;
+        if !global.mutable {
+            bail!(
+                self.offset,
+                "global is immutable: cannot modify it with `global.atomic.rmw.xchg`"
+            );
+        }
+        let ty = global.content_type;
         if !(ty == ValType::I32
             || ty == ValType::I64
             || self.resources.is_subtype(ty, RefType::ANYREF.into()))
@@ -2469,7 +2483,14 @@ where
         _ordering: crate::Ordering,
         global_index: u32,
     ) -> Self::Output {
-        let ty = self.global_type_at(global_index)?.content_type;
+        let global = self.global_type_at(global_index)?;
+        if !global.mutable {
+            bail!(
+                self.offset,
+                "global is immutable: cannot modify it with `global.atomic.rmw.cmpxchg`"
+            );
+        }
+        let ty = global.content_type;
         if !(ty == ValType::I32
             || ty == ValType::I64
             || self.resources.is_subtype(ty, RefType::EQREF.into()))
@@ -3709,7 +3730,7 @@ where
         struct_type_index: u32,
         field_index: u32,
     ) -> Self::Output {
-        self.visit_struct_get_s(struct_type_index, field_index)?;
+        self.visit_struct_get_u(struct_type_index, field_index)?;
         // This instruction has the same type restrictions as the non-`atomic` version.
         debug_assert!(matches!(
             self.struct_field_at(struct_type_index, field_index)?

tests/cli/shared-everything-threads/globals.wast

@@ -380,6 +380,78 @@
   )
   "invalid type: `global.atomic.rmw.*` only allows `i32` and `i64`")
 
+;; Check global.atomic.rmw.* on immutable globals.
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.add seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.sub seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.and seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.or seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.xor seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      global.atomic.rmw.xchg seq_cst $g
+    )
+  )
+  "global is immutable")
+
+(assert_invalid
+  (module
+    (global $g (shared i32) (i32.const 0))
+    (func (result i32)
+      i32.const 1
+      i32.const 2
+      global.atomic.rmw.cmpxchg seq_cst $g
+    )
+  )
+  "global is immutable")
+
 (assert_invalid
   (module
     (func

tests/snapshots/cli/shared-everything-threads/globals.wast.json

@@ -181,35 +181,84 @@
     },
     {
       "type": "assert_invalid",
-      "line": 384,
+      "line": 385,
       "filename": "globals.26.wasm",
       "module_type": "binary",
-      "text": "global index out of bounds"
+      "text": "global is immutable"
     },
     {
       "type": "assert_invalid",
-      "line": 392,
+      "line": 395,
       "filename": "globals.27.wasm",
       "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 405,
+      "filename": "globals.28.wasm",
+      "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 415,
+      "filename": "globals.29.wasm",
+      "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 425,
+      "filename": "globals.30.wasm",
+      "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 435,
+      "filename": "globals.31.wasm",
+      "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 445,
+      "filename": "globals.32.wasm",
+      "module_type": "binary",
+      "text": "global is immutable"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 456,
+      "filename": "globals.33.wasm",
+      "module_type": "binary",
+      "text": "global index out of bounds"
+    },
+    {
+      "type": "assert_invalid",
+      "line": 464,
+      "filename": "globals.34.wasm",
+      "module_type": "binary",
       "text": "invalid type: `global.atomic.rmw.xchg` only allows `i32`, `i64` and subtypes of `anyref`"
     },
     {
       "type": "module",
-      "line": 402,
-      "filename": "globals.28.wasm",
+      "line": 474,
+      "filename": "globals.35.wasm",
       "module_type": "binary"
     },
     {
       "type": "assert_invalid",
-      "line": 412,
-      "filename": "globals.29.wasm",
+      "line": 484,
+      "filename": "globals.36.wasm",
       "module_type": "binary",
       "text": "invalid type: `global.atomic.rmw.cmpxchg` only allows `i32`, `i64` and subtypes of `eqref`"
     },
     {
       "type": "module",
-      "line": 423,
-      "filename": "globals.30.wasm",
+      "line": 495,
+      "filename": "globals.37.wasm",
       "module_type": "binary"
     }
   ]