Repository: bytecodealliance/wasmtime (rust)

Issue:
Cranelift: assertion `left == right` failed: the memory base pointer may be incorrect due to sharing memory
### Steps to Reproduce

I have a binary (`wasm-ld`, which is `lld` from the LLVM distribution) built with `wasi-sdk-22.0` for the `wasm32-wasi-threads` target, with `CFLAGS` of `-pthread` and `LDFLAGS` of `-Wl,--max-memory=4294967296`. It is built with debug symbols using the normal LLVM mechanism (`RelWithDebInfo`). It is too big to include here but I believe the relevant excerpt is:

```
  (memory (;0;) 53 65536 shared)
  (export "memory" (memory 0))
```

I run the binary with `RUST_BACKTRACE=full WASMTIME_BACKTRACE_DETAILS=1 wasmtime -S threads --dir . --dir /tmp -D debug-info -D address-map ./llvm-build/bin/wasm-ld <...arguments that cause a trap...>`.

### Expected Results

A backtrace with Wasm-level symbols is displayed.

### Actual Results

<details>
<summary>An assertion failure in Cranelift.</summary>

```
thread 'main' panicked at /home/whitequark/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-cranelift-20.0.2/src/compiler.rs:559:13:
assertion `left == right` failed: the memory base pointer may be incorrect due to sharing memory
  left: 1
 right: 0
stack backtrace:
   0:     0x55f64b236496 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h402f02d9b4df5fe1
   1:     0x55f64b262c4c - core::fmt::write::h8fddb4828840201f
   2:     0x55f64b23234f - std::io::Write::write_fmt::h38b9b1a6a63dbb61
   3:     0x55f64b236244 - std::sys_common::backtrace::print::h98a1789ae7f04118
   4:     0x55f64b237a3b - std::panicking::default_hook::{{closure}}::hbf9ed991ae15cec9
   5:     0x55f64b237789 - std::panicking::default_hook::h0e9a403389616b5a
   6:     0x55f64b237edd - std::panicking::rust_panic_with_hook::h47d6ea0dd5408232
   7:     0x55f64b237db2 - std::panicking::begin_panic_handler::{{closure}}::h92169a5d672aa955
   8:     0x55f64b236976 - std::sys_common::backtrace::__rust_end_short_backtrace::h78311f5de6c73a58
   9:     0x55f64b237ae4 - rust_begin_unwind
  10:     0x55f649eb0a15 - core::panicking::panic_fmt::h9fcc2d8bb7eb13a8
  11:     0x55f649eb0e8f - core::panicking::assert_failed_inner::h77eb1113bacb0f31
  12:     0x55f649e588af - core::panicking::assert_failed::he44b2686efc08dff
  13:     0x55f64abe2d30 - <wasmtime_cranelift::compiler::Compiler as wasmtime_environ::compile::Compiler>::append_dwarf::h14d11c20fa9e5f82
  14:     0x55f64aafb697 - wasmtime::compile::FunctionIndices::link_and_append_code::hf766901eb87a60b8
  15:     0x55f64aaf4c31 - wasmtime::compile::build_artifacts::h1809dbb6af98174d
  16:     0x55f64ab4360c - core::ops::function::FnOnce::call_once::h8f5620eee6192dd4
  17:     0x55f64ab6bdc5 - wasmtime_cache::ModuleCacheEntry::get_data_raw::h5918a4673179324b
  18:     0x55f64ab4f059 - wasmtime::compile::runtime::<impl wasmtime::compile::code_builder::CodeBuilder>::compile_module::h6cfb67fd1fb024ce
  19:     0x55f649ee4dec - wasmtime_cli::common::RunCommon::load_module::h0243b5cafcaca8ef
  20:     0x55f649fd2f2a - wasmtime_cli::commands::run::RunCommand::execute::h741dadf6ee6383ad
  21:     0x55f649eb9c01 - wasmtime::Wasmtime::execute::h9f6abe47b919fe59
  22:     0x55f649eb5088 - wasmtime::old_cli::main::hbf2a698a610c379d
  23:     0x55f649eb69a3 - std::sys_common::backtrace::__rust_begin_short_backtrace::h2d92647e84e7ad00
  24:     0x55f649eb69bd - std::rt::lang_start::{{closure}}::h3b525ebc181e5daf
  25:     0x55f64b228533 - std::rt::lang_start_internal::hb867f343d27fa61e
  26:     0x55f649ebb4a5 - main
  27:     0x7f85f8f3f24a - __libc_start_call_main
                               at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  28:     0x7f85f8f3f305 - __libc_start_main_impl
                               at ./csu/../csu/libc-start.c:360:3
  29:     0x55f649eb10c1 - _start
  30:                0x0 - <unknown>
```
</details>

### Versions and Environment

Cranelift version or commit: `wasmtime-cranelift-20.0.2`

Operating system: Linux

Architecture: x86_64


Pull request description:
Handle defined shared memories in dwarf processing
This commit resolves an assert in the dwarf generating of core wasm modules when the module has a defined linear memory which is flagged `shared`. This is represented slightly differently in the `VMContext` than owned memories that aren't `shared`, and looks more like an imported memory. With support in #8740 it's now much easier to support this.

Closes #8652

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->


Patch:
diff --git a/crates/cranelift/src/debug.rs b/crates/cranelift/src/debug.rs
index ed9c47310..542a07092 100644
--- a/crates/cranelift/src/debug.rs
+++ b/crates/cranelift/src/debug.rs
@@ -5,8 +5,8 @@ use cranelift_codegen::isa::TargetIsa;
 use object::write::SymbolId;
 use std::collections::HashMap;
 use wasmtime_environ::{
-    DefinedFuncIndex, EntityRef, MemoryIndex, ModuleTranslation, OwnedMemoryIndex, PrimaryMap,
-    PtrSize, StaticModuleIndex, Tunables, VMOffsets,
+    DefinedFuncIndex, DefinedMemoryIndex, EntityRef, MemoryIndex, ModuleTranslation,
+    OwnedMemoryIndex, PrimaryMap, PtrSize, StaticModuleIndex, Tunables, VMOffsets,
 };
 
 /// Memory definition offset in the VMContext structure.
@@ -88,25 +88,21 @@ impl<'a> Compilation<'a> {
             );
 
             let memory_offset = if ofs.num_imported_memories > 0 {
+                let index = MemoryIndex::new(0);
                 ModuleMemoryOffset::Imported {
-                    offset_to_vm_memory_definition: ofs.vmctx_vmmemory_import(MemoryIndex::new(0))
+                    offset_to_vm_memory_definition: ofs.vmctx_vmmemory_import(index)
                         + u32::from(ofs.vmmemory_import_from()),
                     offset_to_memory_base: ofs.ptr.vmmemory_definition_base().into(),
                 }
+            } else if ofs.num_owned_memories > 0 {
+                let index = OwnedMemoryIndex::new(0);
+                ModuleMemoryOffset::Defined(ofs.vmctx_vmmemory_definition_base(index))
             } else if ofs.num_defined_memories > 0 {
-                // The addition of shared memory makes the following assumption,
-                // "owned memory index = 0", possibly false. If the first memory
-                // is a shared memory, the base pointer will not be stored in
-                // the `owned_memories` array. The following code should
-                // eventually be fixed to not only handle shared memories but
-                // also multiple memories.
-                assert_eq!(
-                    ofs.num_defined_memories, ofs.num_owned_memories,
-                    "the memory base pointer may be incorrect due to sharing memory"
-                );
-                ModuleMemoryOffset::Defined(
-                    ofs.vmctx_vmmemory_definition_base(OwnedMemoryIndex::new(0)),
-                )
+                let index = DefinedMemoryIndex::new(0);
+                ModuleMemoryOffset::Imported {
+                    offset_to_vm_memory_definition: ofs.vmctx_vmmemory_pointer(index),
+                    offset_to_memory_base: ofs.ptr.vmmemory_definition_base().into(),
+                }
             } else {
                 ModuleMemoryOffset::None
             };
diff --git a/crates/wasi-preview1-component-adapter/src/descriptors.rs b/crates/wasi-preview1-component-adapter/src/descriptors.rs
index e307d50af..073e4fe83 100644
--- a/crates/wasi-preview1-component-adapter/src/descriptors.rs
+++ b/crates/wasi-preview1-component-adapter/src/descriptors.rs
@@ -1,6 +1,6 @@
 use crate::bindings::wasi::cli::{stderr, stdin, stdout};
 use crate::bindings::wasi::io::streams::{InputStream, OutputStream};
-use crate::{BlockingMode, BumpAlloc, ImportAlloc, State, TrappingUnwrap, WasmStr};
+use crate::{BlockingMode, ImportAlloc, State, TrappingUnwrap, WasmStr};
 use core::cell::{Cell, OnceCell, UnsafeCell};
 use core::mem::MaybeUninit;
 use core::num::NonZeroUsize;
@@ -232,9 +232,10 @@ impl Descriptors {
 
     #[cfg(not(feature = "proxy"))]
     pub unsafe fn get_preopen_path(&self, state: &State, fd: Fd, path: *mut u8, len: usize) {
+        let nth = fd - 3;
         let alloc = ImportAlloc::GetPreopenPath {
-            path: Some(BumpAlloc { base: path, len }),
-            nth: fd - 3,
+            cur: 0,
+            nth,
             alloc: state.temporary_alloc(),
         };
         let (preopens, _) = state.with_import_alloc(alloc, || {
@@ -253,6 +254,12 @@ impl Descriptors {
         for i in 0..preopens.len {
             let preopen = preopens.base.add(i).read();
             drop(preopen.descriptor);
+
+            if (i as u32) != nth {
+                continue;
+            }
+            assert!(preopen.path.len <= len);
+            core::ptr::copy(preopen.path.ptr, path, preopen.path.len);
         }
     }
 
diff --git a/crates/wasi-preview1-component-adapter/src/lib.rs b/crates/wasi-preview1-component-adapter/src/lib.rs
index f41d78273..142d1a3df 100644
--- a/crates/wasi-preview1-component-adapter/src/lib.rs
+++ b/crates/wasi-preview1-component-adapter/src/lib.rs
@@ -190,13 +190,10 @@ pub unsafe extern "C" fn cabi_import_realloc(
     align: usize,
     new_size: usize,
 ) -> *mut u8 {
-    if !old_ptr.is_null() || old_size != 0 {
-        unreachable!();
-    }
     let mut ptr = null_mut::<u8>();
     State::with(|state| {
         let mut alloc = state.import_alloc.replace(ImportAlloc::None);
-        ptr = alloc.alloc(align, new_size);
+        ptr = alloc.alloc(old_ptr, old_size, align, new_size);
         state.import_alloc.set(alloc);
         Ok(())
     });
@@ -282,11 +279,15 @@ enum ImportAlloc {
     /// An allocator specifically for getting the nth string allocation used
     /// for preopens.
     ///
-    /// This will allocate the `nth` string allocation within the `path`
-    /// allocator. Otherwise all string allocations are discarded but
-    /// temoprarily placed into `alloc`.
+    /// This will allocate everything into `alloc`. All strings other than the
+    /// `nth` string, however, will be discarded (the allocator's state is reset
+    /// after the allocation). This means that the pointer returned for the
+    /// `nth` string will be retained in `alloc` while all others will be
+    /// discarded.
+    ///
+    /// The `cur` count starts at 0 and counts up per-string.
     GetPreopenPath {
-        path: Option<BumpAlloc>,
+        cur: u32,
         nth: u32,
         alloc: BumpAlloc,
     },
@@ -298,7 +299,78 @@ enum ImportAlloc {
 
 impl ImportAlloc {
     /// To be used by cabi_import_realloc only!
-    unsafe fn alloc(&mut self, align: usize, size: usize) -> *mut u8 {
+    unsafe fn alloc(
+        &mut self,
+        old_ptr: *mut u8,
+        old_size: usize,
+        align: usize,
+        size: usize,
+    ) -> *mut u8 {
+        // This is ... a hack. This is a hack in subtle ways that is quite
+        // brittle and may break over time. There's only one case for the
+        // `realloc`-like-behavior in the canonical ABI and that's when the host
+        // is transferring a string to the guest and the host has a different
+        // string encoding. For example JS uses utf-16 (ish) and Rust/WASIp1 use
+        // utf-8. That means that when this adapter is used with a JS host
+        // realloc behavior may be triggered in which case `old_ptr` may not be
+        // null.
+        //
+        // In the case that `old_ptr` may not be null we come to the first
+        // brittle assumption: it's assumed that this is shrinking memory. In
+        // the canonical ABI overlarge allocations are made originally and then
+        // shrunk afterwards once encoding is finished. This means that the
+        // first allocation is too big and the `realloc` call is shrinking
+        // memory. This assumption may be violated in the future if the
+        // canonical ABI is updated to handle growing strings in addition to
+        // shrinking strings. (e.g. starting with an assume-ascii path and then
+        // falling back to an ok-needs-more-space path for larger unicode code
+        // points).
+        //
+        // This comes to the second brittle assumption, nothing happens here
+        // when a shrink happens. This is brittle for each of the cases below,
+        // enumerated here:
+        //
+        // * For `OneAlloc` this isn't the end of the world. That's already
+        //   asserting that only a single string is allocated. Returning the
+        //   original pointer keeps the pointer the same and the host will keep
+        //   track of the appropriate length. In this case the final length is
+        //   read out of the return value of a function, meaning that everything
+        //   actually works out here.
+        //
+        // * For `CountAndDiscardStrings` we're relying on the fact that
+        //   this is only used for `environ_sizes_get` and `args_sizes_get`. In
+        //   both situations we're actually going to return an "overlarge"
+        //   return value for the size of arguments and return values. By
+        //   assuming memory shrinks after the first allocation the return value
+        //   of `environ_sizes_get` and `args_sizes_get` will be the overlong
+        //   approximation for all strings. That means that the final exact size
+        //   won't be what's returned. This ends up being ok because technically
+        //   nothing about WASI says that those blocks have to be exact-sized.
+        //   In our case we're (ab)using that to force the caller to make an
+        //   overlarge return area which we'll allocate into. All-in-all we
+        //   don't track the shrink request and ignore the size.
+        //
+        // * For `SeparateStringsAndPointers` it's similar to the previous case
+        //   except the weird part is that the caller is providing the
+        //   argument/env space buffer to write into. It's over-large because of
+        //   the case of `CountAndDiscardStrings` above, but we'll exploit that
+        //   here and end up having space between all the arguments. Technically
+        //   WASI doesn't say all the strings have to be adjacent, so this
+        //   should work out in practice.
+        //
+        // * Finally for `GetPreopenPath` this works out only insofar that the
+        //   `State::temporary_alloc` space is used to store the path. The
+        //   WASI-provided buffer is precisely sized, not overly large, meaning
+        //   that we're forced to copy from `temporary_alloc` into the
+        //   destination buffer for this WASI call.
+        //
+        // Basically it's a case-by-case basis here that enables ignoring
+        // shrinking return calls here. Not robust.
+        if !old_ptr.is_null() {
+            assert!(old_size > size);
+            assert_eq!(align, 1);
+            return old_ptr;
+        }
         match self {
             ImportAlloc::OneAlloc(alloc) => {
                 let ret = alloc.alloc(align, size);
@@ -323,23 +395,21 @@ impl ImportAlloc {
                     alloc.alloc(align, size)
                 }
             }
-            ImportAlloc::GetPreopenPath { path, nth, alloc } => {
+            ImportAlloc::GetPreopenPath { cur, nth, alloc } => {
                 if align == 1 {
-                    if *nth == 0 {
-                        if let Some(a) = path {
-                            let ret = a.alloc(align, size);
-                            *path = None;
-                            return ret;
-                        }
+                    let real_alloc = *nth == *cur;
+                    if real_alloc {
+                        alloc.alloc(align, size)
                     } else {
-                        *nth -= 1;
+                        alloc.clone().alloc(align, size)
                     }
-                    alloc.clone().alloc(align, size)
                 } else {
                     alloc.alloc(align, size)
                 }
             }
-            ImportAlloc::None => unreachable!("no allocator configured"),
+            ImportAlloc::None => {
+                unreachable!("no allocator configured")
+            }
         }
     }
 }
@@ -2623,7 +2693,7 @@ const fn temporary_data_size() -> usize {
     }
 
     // Remove miscellaneous metadata also stored in state.
-    let misc = if cfg!(feature = "proxy") { 9 } else { 12 };
+    let misc = if cfg!(feature = "proxy") { 8 } else { 10 };
     start -= misc * size_of::<usize>();
 
     // Everything else is the `command_data` allocation.


Test patch:
diff --git a/crates/test-programs/artifacts/build.rs b/crates/test-programs/artifacts/build.rs
index 0fa76093e..20209eaf6 100644
--- a/crates/test-programs/artifacts/build.rs
+++ b/crates/test-programs/artifacts/build.rs
@@ -90,7 +90,7 @@ fn build_and_generate_tests() {
         }
 
         // Generate a component from each test.
-        if kind == "nn" || target == "dwarf_imported_memory" {
+        if kind == "nn" || target == "dwarf_imported_memory" || target == "dwarf_shared_memory" {
             continue;
         }
         let adapter = match target.as_str() {
diff --git a/crates/test-programs/build.rs b/crates/test-programs/build.rs
index 734cbddbb..8f055264c 100644
--- a/crates/test-programs/build.rs
+++ b/crates/test-programs/build.rs
@@ -1,4 +1,6 @@
 fn main() {
     println!("cargo:rustc-link-arg-bin=dwarf_imported_memory=--import-memory");
     println!("cargo:rustc-link-arg-bin=dwarf_imported_memory=--export-memory");
+    println!("cargo:rustc-link-arg-bin=dwarf_shared_memory=--no-check-features");
+    println!("cargo:rustc-link-arg-bin=dwarf_shared_memory=--shared-memory");
 }
diff --git a/crates/test-programs/src/bin/dwarf_shared_memory.rs b/crates/test-programs/src/bin/dwarf_shared_memory.rs
new file mode 100644
index 000000000..fe4ca3784
--- /dev/null
+++ b/crates/test-programs/src/bin/dwarf_shared_memory.rs
@@ -0,0 +1,1 @@
+include!("./dwarf_simple.rs");
diff --git a/crates/wasi-http/tests/all/main.rs b/crates/wasi-http/tests/all/main.rs
index 68de94195..4f86af889 100644
--- a/crates/wasi-http/tests/all/main.rs
+++ b/crates/wasi-http/tests/all/main.rs
@@ -535,7 +535,7 @@ async fn wasi_http_without_port() -> Result<()> {
         .method(http::Method::GET)
         .uri("https://httpbin.org/get");
 
-    let response = run_wasi_http(
+    let _response: hyper::Response<_> = run_wasi_http(
         test_programs_artifacts::API_PROXY_FORWARD_REQUEST_COMPONENT,
         req.body(body::empty())?,
         None,
@@ -543,7 +543,10 @@ async fn wasi_http_without_port() -> Result<()> {
     )
     .await??;
 
-    assert_eq!(response.status(), StatusCode::OK);
+    // NB: don't test the actual return code of `response`. This is testing a
+    // live http request against a live server and things happen. If we got this
+    // far it's already successful that the request was made and the lack of
+    // port in the URI was handled.
 
     Ok(())
 }
diff --git a/tests/all/debug/lldb.rs b/tests/all/debug/lldb.rs
index dadf8a154..ee347a162 100644
--- a/tests/all/debug/lldb.rs
+++ b/tests/all/debug/lldb.rs
@@ -370,4 +370,10 @@ check: exited with status = 0
             &["--preload=env=./tests/all/debug/satisfy_memory_import.wat"],
         )
     }
+
+    #[test]
+    #[ignore]
+    fn dwarf_shared_memory() -> Result<()> {
+        test_dwarf_simple(DWARF_SHARED_MEMORY, &[])
+    }
 }