Fix bounds checks in FACT's string_to_compact method (bytecodealliance#13016)

We need to bounds check the source byte length, not the number of code units.

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

Author: alexcrichton

crates/environ/src/fact/trampoline.rs

@@ -1759,7 +1759,6 @@ impl<'a, 'b> Compiler<'a, 'b> {
         dst_enc: FE,
     ) -> WasmString<'c> {
         assert!(dst_enc.width() >= src_enc.width());
-        self.validate_string_length(src, dst_enc);
 
         let src_mem_opts = {
             match &src.opts.data_model {
@@ -1774,22 +1773,8 @@ impl<'a, 'b> Compiler<'a, 'b> {
             }
         };
 
-        // Calculate the source byte length given the size of each code
-        // unit. Note that this shouldn't overflow given
-        // `validate_string_length` above.
-        let mut src_byte_len_tmp = None;
-        let src_byte_len = if src_enc.width() == 1 {
-            src.len.idx
-        } else {
-            assert_eq!(src_enc.width(), 2);
-            self.instruction(LocalGet(src.len.idx));
-            self.ptr_uconst(src_mem_opts, 1);
-            self.ptr_shl(src_mem_opts);
-            let tmp = self.local_set_new_tmp(src.opts.data_model.unwrap_memory().ptr());
-            let ret = tmp.idx;
-            src_byte_len_tmp = Some(tmp);
-            ret
-        };
+        let (src_byte_len_tmp, src_byte_len) =
+            self.source_string_byte_len(src, src_enc, src_mem_opts);
 
         // Convert the source code units length to the destination byte
         // length type.
@@ -1851,6 +1836,39 @@ impl<'a, 'b> Compiler<'a, 'b> {
 
         dst
     }
+
+    /// Calculate the source byte length given the size of each code
+    /// unit.
+    ///
+    /// Returns an optional temporary local if it was needed, which the caller
+    /// needs to deallocate with `free_temp_local`. Additionally returns the
+    /// index of the local which contains the byte length of the string, which
+    /// may point to the temporary local passed in.
+    fn source_string_byte_len(
+        &mut self,
+        src: &WasmString<'_>,
+        src_enc: FE,
+        src_mem_opts: &LinearMemoryOptions,
+    ) -> (Option<TempLocal>, u32) {
+        self.validate_string_length(src, src_enc);
+
+        if src_enc.width() == 1 {
+            (None, src.len.idx)
+        } else {
+            assert_eq!(src_enc.width(), 2);
+
+            // Note that this shouldn't overflow given `validate_string_length`
+            // above.
+            self.instruction(LocalGet(src.len.idx));
+            self.ptr_uconst(src_mem_opts, 1);
+            self.ptr_shl(src_mem_opts);
+            let tmp = self.local_set_new_tmp(src.opts.data_model.unwrap_memory().ptr());
+
+            let idx = tmp.idx;
+            (Some(tmp), idx)
+        }
+    }
+
     // Corresponding function for `store_string_to_utf8` in the spec.
     //
     // This translation works by possibly performing a number of
@@ -2252,7 +2270,9 @@ impl<'a, 'b> Compiler<'a, 'b> {
             DataModel::LinearMemory(opts) => opts,
         };
 
-        self.validate_string_length(src, src_enc);
+        let (src_byte_len_tmp, src_byte_len) =
+            self.source_string_byte_len(src, src_enc, src_mem_opts);
+
         self.convert_src_len_to_dst(src.len.idx, src_mem_opts.ptr(), dst_mem_opts.ptr());
         let dst_len = self.local_tee_new_tmp(dst_mem_opts.ptr());
         let dst_byte_len = self.local_set_new_tmp(dst_mem_opts.ptr());
@@ -2265,7 +2285,7 @@ impl<'a, 'b> Compiler<'a, 'b> {
             }
         };
 
-        self.validate_string_inbounds(src, src.len.idx);
+        self.validate_string_inbounds(src, src_byte_len);
         self.validate_string_inbounds(&dst, dst_byte_len.idx);
 
         // Perform the initial latin1 transcode. This returns the number of
@@ -2384,6 +2404,9 @@ impl<'a, 'b> Compiler<'a, 'b> {
 
         self.free_temp_local(src_len_tmp);
         self.free_temp_local(dst_byte_len);
+        if let Some(tmp) = src_byte_len_tmp {
+            self.free_temp_local(tmp);
+        }
 
         dst
     }

tests/misc_testsuite/component-model/string-transcode-invalid.wast

@@ -0,0 +1,62 @@
+;;! multi_memory = true
+
+;; Transcode a utf16 string to latin1+utf16, but the original string is
+;; out-of-bounds. Should report a first-class error.
+(component
+  (component $dst
+    (core module $m
+      (func (export "recv") (param i32 i32))
+      (func (export "realloc") (param i32 i32 i32 i32) (result i32) i32.const 0)
+      (memory (export "memory") 1)
+    )
+    (core instance $i (instantiate $m))
+    (func (export "recv") (param "a" string)
+      (canon lift (core func $i "recv") (realloc (func $i "realloc")) (memory $i "memory")
+        string-encoding=latin1+utf16)
+    )
+  )
+
+  ;; Source component: uses utf16 encoding.
+  ;; Passes a string placed near the END of linear memory to $dst.
+  (component $src
+    (import "recv" (func $recv (param "a" string)))
+    (core module $libc
+      (memory (export "memory") 1)  ;; 1 page = 65536 bytes
+      (func (export "realloc") (param i32 i32 i32 i32) (result i32) i32.const 0)
+    )
+    (core instance $libc (instantiate $libc))
+    ;; canon lower with utf16 — the source side of the mismatch.
+    (core func $recv_lowered (canon lower (func $recv) string-encoding=utf16 (memory $libc "memory")))
+    (core module $m
+      (import "" "" (func $recv (param i32 i32)))
+      (import "libc" "memory" (memory 0))
+      (func (export "run")
+        ;; Write 8 UTF-16 code units (16 bytes) of 'A' (U+0041) at the end of memory.
+        ;; Offsets 65520–65535: exactly fills to the last byte of the page.
+        (i32.store (i32.const 65520) (i32.const 0x00410041))
+        (i32.store (i32.const 65524) (i32.const 0x00410041))
+        (i32.store (i32.const 65528) (i32.const 0x00410041))
+        (i32.store (i32.const 65532) (i32.const 0x00410041))
+
+        ;; Pass ptr=65520, len=10 CODE UNITS (not bytes).
+        ;; We wrote 8 code units but claim 10 — the extra 2 are past end of memory.
+        (call $recv (i32.const 65520) (i32.const 10))
+      )
+    )
+    (core instance $i (instantiate $m
+      (with "" (instance (export "" (func $recv_lowered))))
+      (with "libc" (instance $libc))
+    ))
+    (func (export "run")
+      (canon lift (core func $i "run"))
+    )
+  )
+
+  ;; Wire the components together and run.
+  (instance $dst_inst (instantiate $dst))
+  (instance $i (instantiate $src (with "recv" (func $dst_inst "recv"))))
+
+  (export "run" (func $i "run"))
+)
+
+(assert_trap (invoke "run") "string content out-of-bounds")