Test wasm32 without js - #6283
Conversation
Sorry, something went wrong.
WalkthroughThe PR replaces the broken wasm test infrastructure by relocating the wasm crate from wasm/wasm-unknown-test/ to a new structured example project at example_projects/wasm32_without_js/. It introduces a wasm producer crate (rustpython-without-js) and a wasm runtime consumer (wasm-runtime) using Wasmer, along with CI workflow updates. Changes
Sequence DiagramsequenceDiagram
participant Host as Host Runtime<br>(main.rs)
participant WasmModule as WASM Module<br>(eval)
participant RustPython as RustPython VM
participant KVStore as KV Store
Host->>Host: Load WASM module from path
Host->>Host: Initialize KV store with code
Host->>WasmModule: Call eval(code_ptr, code_len)
activate WasmModule
WasmModule->>RustPython: Create Interpreter
RustPython->>RustPython: Execute Python expr "1 + 3"
RustPython-->>WasmModule: Result repr
WasmModule->>Host: Call print(msg_ptr, msg_len)
activate Host
Host->>Host: Read string from WASM memory
Host->>Host: Print to stdout
Host-->>WasmModule: return 0
deactivate Host
WasmModule-->>Host: return 0 (success)
deactivate WasmModule
Host->>Host: Report result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)example_projects/wasm32_without_js/wasm-runtime/.gitignore (1)📜 Review detailsexample_projects/wasm32_without_js/rustpython-without-js/Cargo.toml (1)1-4: Confirm whether the !wasm/rustpython.wasm exception is still needed
This override looks like a leftover from the previous layout; the new example seems to produce rustpython-without-js/target/.../rustpython_without_js.wasm instead. If wasm/rustpython.wasm is no longer used, consider removing or updating this exception to avoid confusion.
example_projects/wasm32_without_js/wasm-runtime/README.md (1)13-15: Consider dropping empty [workspace] / [patch.crates-io] sections
If this crate is meant to live inside the existing top-level workspace and you’re not patching crates, these sections can be removed to reduce manifest noise and avoid accidentally creating a nested workspace.
example_projects/wasm32_without_js/wasm-runtime/src/main.rs (2)3-11: Clarify the wat2wasm prerequisite or remove it
The README asks to “install wat2wasm and rust”, but the example usage only shows:
cargo run --release <wasm binary>If the current flow always consumes a .wasm produced by cargo build (as in the other README) and no .wat is involved anymore, consider either:
- Explaining where wat2wasm fits into the workflow, or
- Dropping the wat2wasm prerequisite to keep the instructions focused.
56-72: Clarify the intent of commented code.
The get_code function is fully commented out. Should this be removed entirely, or is it planned for future use? If it's dead code, consider removing it to improve maintainability.
95-100: Clarify hard-coded Python snippet purpose.
The runtime initializes the KV store with hard-coded Python code ("a=10;b='str';f'{a}{b}'"). For an example/demo, this might be appropriate, but consider:
- Documenting why this code is hard-coded
- Making it configurable via command-line or environment variable
- Adding a comment explaining what this snippet is testing
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 916d3ba and 1a92d4f.
📒 Files selected for processing (11)- .github/workflows/ci.yaml (1 hunks)
- example_projects/wasm32_without_js/.gitignore (1 hunks)
- example_projects/wasm32_without_js/README.md (1 hunks)
- example_projects/wasm32_without_js/rustpython-without-js/Cargo.toml (1 hunks)
- example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (1 hunks)
- example_projects/wasm32_without_js/wasm-runtime/.gitignore (1 hunks)
- example_projects/wasm32_without_js/wasm-runtime/Cargo.toml (1 hunks)
- example_projects/wasm32_without_js/wasm-runtime/README.md (1 hunks)
- example_projects/wasm32_without_js/wasm-runtime/src/main.rs (1 hunks)
- wasm/wasm-unknown-test/Cargo.toml (0 hunks)
- wasm/wasm-unknown-test/src/lib.rs (0 hunks)
- wasm/wasm-unknown-test/src/lib.rs
- wasm/wasm-unknown-test/Cargo.toml
Learnt from: arihant2math Repo: RustPython/RustPython PR: 5790 File: build.rs:2-2 Timestamp: 2025-06-28T16:31:03.991Z Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
- example_projects/wasm32_without_js/.gitignore
example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (3)example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (1)
- kv_get (3-4)
- kv_put (7-7)
- print (10-10)
example_projects/wasm32_without_js/wasm-runtime/src/main.rs (3)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- kv_get (13-41)
- kv_put (44-54)
- print (75-85)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: auto_format
example_projects/wasm32_without_js/.gitignore (1)example_projects/wasm32_without_js/rustpython-without-js/Cargo.toml (1)1-2: Standard Rust workspace ignore patterns look appropriate.
The patterns correctly ignore build artifacts (target/) and lock files (Cargo.lock) at the crate level within the workspace, which aligns with typical Rust project conventions for example workspaces.
Optional verification: Confirm that the workspace structure matches the pattern depth (one level of subdirectories). If the workspace root itself generates target/ or Cargo.lock artifacts that need ignoring, add corresponding root-level patterns (target/ and Cargo.lock without the */ prefix). If those are handled by a parent .gitignore in the RustPython repository root, no further action is needed.
example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (1)1-4: Verify that using edition = "2024" matches your toolchain and workspace
This crate is set to Rust 2024 edition. Please confirm CI and local toolchains actually support this edition and that it’s consistent with the rest of the workspace; otherwise you may want to stick to the edition used by the main crates (commonly 2021 today) to avoid surprise compilation issues.
.github/workflows/ci.yaml (1)51-60: __getrandom_v03_custom has attribute and behavioral issues
The good news: the current extern "Rust" ABI is correct per getrandom 0.3 documentation.
The remaining issues:
- #[unsafe(no_mangle)] is invalid syntax; should be #[no_mangle].
- Returns Ok(()) without writing to dest, leaving the buffer uninitialized (UB).
Fix:
-#[unsafe(no_mangle)] -unsafe extern "Rust" fn __getrandom_v03_custom( +#[no_mangle] +unsafe extern "Rust" fn __getrandom_v03_custom( _dest: *mut u8, _len: usize, ) -> Result<(), getrandom::Error> { - // Err(getrandom::Error::UNSUPPORTED) - - // WARNING: This function **MUST** perform proper getrandom - Ok(()) + // TODO: implement proper randomness for wasm32 + Err(getrandom::Error::UNSUPPORTED) }Likely an incorrect or invalid review comment.
example_projects/wasm32_without_js/wasm-runtime/src/main.rs (1)412-412: Verify build profile mismatch is intentional.
The wasm-runtime is built with --release but loads the wasm artifact from the debug/ directory. This mismatch may be intentional for testing, but please confirm this is the desired behavior.
example_projects/wasm32_without_js/wasm-runtime/Cargo.toml (1)1-9: LGTM!
The structure and imports are well-organized. The Ctx struct appropriately uses Option<Memory> since the memory reference is established after WASM module instantiation.
7-7: wasmer 6.1.0 is current and secure.
Verification confirms that version 6.1.0 is the latest available version on crates.io. The only known security vulnerability (symlink sandbox bypass) affects versions <= 4.3.1 and does not impact 6.1.0. No action required.
Sorry, something went wrong.
| run: | | ||
| cd wasm/wasm-unknown-test | ||
| cargo build --release --verbose | ||
| if wasm-objdump -xj Import target/wasm32-unknown-unknown/release/wasm_unknown_test.wasm; then | ||
| cd example_projects/wasm32_without_js | ||
| cargo build --manifest-path rustpython-without-js/Cargo.toml | ||
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | ||
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >2 | ||
| fi |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Fix shell redirect syntax error.
Line 410 uses >2 which will create a file named "2" instead of redirecting to stderr. The correct syntax is >&2.
Apply this diff:
if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then
- echo "ERROR: wasm32-unknown module expects imports from the host environment" >2
+ echo "ERROR: wasm32-unknown module expects imports from the host environment" >&2
fi‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| cd wasm/wasm-unknown-test | |
| cargo build --release --verbose | |
| if wasm-objdump -xj Import target/wasm32-unknown-unknown/release/wasm_unknown_test.wasm; then | |
| cd example_projects/wasm32_without_js | |
| cargo build --manifest-path rustpython-without-js/Cargo.toml | |
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | |
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >2 | |
| fi | |
| run: | | |
| cd example_projects/wasm32_without_js | |
| cargo build --manifest-path rustpython-without-js/Cargo.toml | |
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | |
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >&2 | |
| fi |
.github/workflows/ci.yaml around lines 406 to 411: the shell redirection uses `>2` which creates a file named "2" instead of redirecting to stderr; change the redirection to `>&2` so the error message is written to stderr (i.e., replace the `echo "ERROR: ... " >2` with `echo "ERROR: ... " >&2`).
Sorry, something went wrong.
| To test, build rustpython to wasm32-unknown-unknown target first. | ||
|
|
||
| ```shell | ||
| cargo build --manifest-path rustpython-without-js/Cargo.toml | ||
| ``` | ||
|
|
||
| Then there will be `rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm` file. | ||
|
|
||
| Now we can run the wasm file with wasm runtime: | ||
|
|
||
| ```shell | ||
| cargo run --release --manifest-path wasm-runtime/Cargo.toml rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm | ||
| ``` |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Add the explicit wasm32 target to the build command
The text says “build … to wasm32-unknown-unknown target” and then references target/wasm32-unknown-unknown/..., but the sample command doesn’t pass a --target:
cargo build --manifest-path rustpython-without-js/Cargo.tomlTo match the described output path, this should be:
-cargo build --manifest-path rustpython-without-js/Cargo.toml
+cargo build --target wasm32-unknown-unknown --manifest-path rustpython-without-js/Cargo.tomlOtherwise users on a default host toolchain will not get the .wasm artifact at the documented location.
🤖 Prompt for AI AgentsIn example_projects/wasm32_without_js/README.md around lines 3 to 15, the build command omits the explicit wasm32 target so the output path shown will not be produced on a default host toolchain; update the documented cargo build command to include --target wasm32-unknown-unknown so the .wasm artifact appears under target/wasm32-unknown-unknown/... (also consider adding the same --target to any run command examples that reference that path).
Sorry, something went wrong.
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | ||
| // let src = unsafe { std::slice::from_raw_parts(s, l) }; | ||
| // let src = std::str::from_utf8(src).unwrap(); | ||
| // TODO: use src | ||
| let src = "1 + 3"; | ||
|
|
||
| // 2. Execute Python code | ||
| let interpreter = Interpreter::without_stdlib(Default::default()); | ||
| let result = interpreter.enter(|vm| { | ||
| let scope = vm.new_scope_with_builtins(); | ||
| let res = match vm.run_block_expr(scope, src) { | ||
| Ok(val) => val, | ||
| Err(_) => return Err(-1), // Python execution error | ||
| }; | ||
| let repr_str = match res.repr(vm) { | ||
| Ok(repr) => repr.as_str().to_string(), | ||
| Err(_) => return Err(-1), // Failed to get string representation | ||
| }; | ||
| Ok(repr_str) | ||
| }); | ||
| let result = match result { | ||
| Ok(r) => r, | ||
| Err(code) => return code, | ||
| }; | ||
|
|
||
| let msg = format!("eval result: {result}"); | ||
|
|
||
| unsafe { | ||
| print( | ||
| result.as_str().as_ptr() as usize as i32, | ||
| result.len() as i32, | ||
| ) | ||
| }; | ||
|
|
||
| 0 | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Fix no_mangle attribute, use the eval input, and print the full message
There are a few issues in this block:
- #[unsafe(no_mangle)] is not a valid attribute and will fail to compile.
- eval ignores its s/l inputs and always runs "1 + 3", which doesn’t match the documented eval(code_ptr, code_len) contract.
- You build msg = "eval result: {result}" but pass result (without the prefix) to print.
A minimal fix that keeps the current structure but makes it usable would look like:
-#[unsafe(no_mangle)]
-pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 {
- // let src = unsafe { std::slice::from_raw_parts(s, l) };
- // let src = std::str::from_utf8(src).unwrap();
- // TODO: use src
- let src = "1 + 3";
+#[no_mangle]
+pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 {
+ let src_bytes = std::slice::from_raw_parts(s, l);
+ let src = match std::str::from_utf8(src_bytes) {
+ Ok(s) => s,
+ Err(_) => return -1, // invalid UTF‑8 input
+ };
@@
- let msg = format!("eval result: {result}");
-
- unsafe {
- print(
- result.as_str().as_ptr() as usize as i32,
- result.len() as i32,
- )
- };
+ let msg = format!("eval result: {result}");
+
+ unsafe {
+ print(
+ msg.as_ptr() as usize as i32,
+ msg.len() as i32,
+ )
+ };This keeps the ABI, actually uses the caller‑provided code, and prints the prefixed message, while preserving your simple i32 error convention.
📝 Committable suggestion‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[unsafe(no_mangle)] | |
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | |
| // let src = unsafe { std::slice::from_raw_parts(s, l) }; | |
| // let src = std::str::from_utf8(src).unwrap(); | |
| // TODO: use src | |
| let src = "1 + 3"; | |
| // 2. Execute Python code | |
| let interpreter = Interpreter::without_stdlib(Default::default()); | |
| let result = interpreter.enter(|vm| { | |
| let scope = vm.new_scope_with_builtins(); | |
| let res = match vm.run_block_expr(scope, src) { | |
| Ok(val) => val, | |
| Err(_) => return Err(-1), // Python execution error | |
| }; | |
| let repr_str = match res.repr(vm) { | |
| Ok(repr) => repr.as_str().to_string(), | |
| Err(_) => return Err(-1), // Failed to get string representation | |
| }; | |
| Ok(repr_str) | |
| }); | |
| let result = match result { | |
| Ok(r) => r, | |
| Err(code) => return code, | |
| }; | |
| let msg = format!("eval result: {result}"); | |
| unsafe { | |
| print( | |
| result.as_str().as_ptr() as usize as i32, | |
| result.len() as i32, | |
| ) | |
| }; | |
| 0 | |
| } | |
| #[no_mangle] | |
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | |
| let src_bytes = std::slice::from_raw_parts(s, l); | |
| let src = match std::str::from_utf8(src_bytes) { | |
| Ok(s) => s, | |
| Err(_) => return -1, // invalid UTF‑8 input | |
| }; | |
| // 2. Execute Python code | |
| let interpreter = Interpreter::without_stdlib(Default::default()); | |
| let result = interpreter.enter(|vm| { | |
| let scope = vm.new_scope_with_builtins(); | |
| let res = match vm.run_block_expr(scope, src) { | |
| Ok(val) => val, | |
| Err(_) => return Err(-1), // Python execution error | |
| }; | |
| let repr_str = match res.repr(vm) { | |
| Ok(repr) => repr.as_str().to_string(), | |
| Err(_) => return Err(-1), // Failed to get string representation | |
| }; | |
| Ok(repr_str) | |
| }); | |
| let result = match result { | |
| Ok(r) => r, | |
| Err(code) => return code, | |
| }; | |
| let msg = format!("eval result: {result}"); | |
| unsafe { | |
| print( | |
| msg.as_ptr() as usize as i32, | |
| msg.len() as i32, | |
| ) | |
| }; | |
| 0 | |
| } |
In example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs around
lines 13 to 49, fix three issues: change the invalid attribute to #[no_mangle]
(keep the function as pub unsafe extern "C" fn eval(...)), actually read the
caller-provided code by creating a slice from_raw_parts(s, l) and converting it
to &str (return an error code on null/utf8 failure), and build the full message
using the formatted prefix (e.g. "eval result: {repr}") then pass that message's
pointer and length to print (cast pointer/len to the i32 ABI types you use) so
the printed output contains the prefix and the evaluated result.
Sorry, something went wrong.
| [package] | ||
| name = "wasm-runtime" | ||
| version = "0.1.0" | ||
| edition = "2024" |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Fix invalid Rust edition.
Edition 2024 does not exist. The latest stable Rust edition is 2021. This will cause a compilation failure.
Apply this diff to fix:
-edition = "2024"
+edition = "2021"‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| edition = "2024" | |
| edition = "2021" |
In example_projects/wasm32_without_js/wasm-runtime/Cargo.toml around line 4, the Cargo.toml incorrectly sets edition = "2024"; change it to the latest valid Rust edition by replacing the value with "2021" so the crate compiles successfully.
Sorry, something went wrong.
| fn kv_get(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut key = vec![0u8; kl as usize]; | ||
| if c.mem | ||
| .as_ref() | ||
| .unwrap() | ||
| .view(&s) | ||
| .read(kp as u64, &mut key) | ||
| .is_err() | ||
| { | ||
| return -1; | ||
| } | ||
| match c.kv.get(&key) { | ||
| Some(val) => { | ||
| let len = val.len().min(vl as usize); | ||
| if c.mem | ||
| .as_ref() | ||
| .unwrap() | ||
| .view(&s) | ||
| .write(vp as u64, &val[..len]) | ||
| .is_err() | ||
| { | ||
| return -1; | ||
| } | ||
| len as i32 | ||
| } | ||
| None => 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Replace unwrap() with proper error handling.
Lines 18 and 30 call unwrap() on c.mem.as_ref(), which will panic if the memory reference is None. While this should never happen if the module is correctly initialized, it creates a potential panic point.
Consider using expect() with a descriptive message or returning an error code:
fn kv_get(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut key = vec![0u8; kl as usize];
- if c.mem
- .as_ref()
- .unwrap()
- .view(&s)
+ let mem = c.mem.as_ref().expect("memory not initialized");
+ if mem.view(&s)
.read(kp as u64, &mut key)
.is_err()
{
return -1;
}
match c.kv.get(&key) {
Some(val) => {
let len = val.len().min(vl as usize);
- if c.mem
- .as_ref()
- .unwrap()
- .view(&s)
+ if mem.view(&s)
.write(vp as u64, &val[..len])
.is_err()
{
return -1;
}
len as i32
}
None => 0,
}
}In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 13 to 41, replace the two uses of c.mem.as_ref().unwrap() with safe error handling: check whether c.mem.as_ref() is Some and if not return an appropriate error code (e.g. -1) or use expect() with a clear message if you prefer a hard failure; then use the unwrapped reference afterward (or bind it to a variable) so the read/write calls operate on a guaranteed memory reference and avoid potential panics.
Sorry, something went wrong.
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut key = vec![0u8; kl as usize]; | ||
| let mut val = vec![0u8; vl as usize]; | ||
| let m = c.mem.as_ref().unwrap().view(&s); | ||
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | ||
| return -1; | ||
| } | ||
| c.kv.insert(key, val); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Replace unwrap() with proper error handling.
Line 48 calls unwrap() on c.mem.as_ref(), which creates a potential panic point if memory is not initialized.
Apply similar fix as suggested for kv_get:
fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut key = vec![0u8; kl as usize];
let mut val = vec![0u8; vl as usize];
- let m = c.mem.as_ref().unwrap().view(&s);
+ let m = c.mem.as_ref().expect("memory not initialized").view(&s);
if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() {
return -1;
}
c.kv.insert(key, val);
0
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut key = vec![0u8; kl as usize]; | |
| let mut val = vec![0u8; vl as usize]; | |
| let m = c.mem.as_ref().unwrap().view(&s); | |
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | |
| return -1; | |
| } | |
| c.kv.insert(key, val); | |
| 0 | |
| } | |
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut key = vec![0u8; kl as usize]; | |
| let mut val = vec![0u8; vl as usize]; | |
| let m = c.mem.as_ref().expect("memory not initialized").view(&s); | |
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | |
| return -1; | |
| } | |
| c.kv.insert(key, val); | |
| 0 | |
| } |
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 44 to 54, replace the direct unwrap() on c.mem.as_ref() with safe handling: check whether c.mem is Some and if not return an error code (e.g., -1) like kv_get does; if Some(mem) obtain the memory view (let m = mem.view(&s)) and continue with the existing reads and inserts, preserving the current read error checks and returning -1 on failure and 0 on success.
Sorry, something went wrong.
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut msg = vec![0u8; l as usize]; | ||
| let m = c.mem.as_ref().unwrap().view(&s); | ||
| if m.read(p as u64, &mut msg).is_err() { | ||
| return -1; | ||
| } | ||
| let s = std::str::from_utf8(&msg).expect("print got non-utf8 str"); | ||
| println!("{s}"); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Improve error handling for memory access and UTF-8 validation.
Two issues:
- Line 78: unwrap() on c.mem.as_ref() creates a panic point
- Line 82: expect() will panic if the WASM module passes non-UTF8 data
Consider this more robust approach:
fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut msg = vec![0u8; l as usize];
- let m = c.mem.as_ref().unwrap().view(&s);
+ let m = c.mem.as_ref().expect("memory not initialized").view(&s);
if m.read(p as u64, &mut msg).is_err() {
return -1;
}
- let s = std::str::from_utf8(&msg).expect("print got non-utf8 str");
- println!("{s}");
- 0
+ match std::str::from_utf8(&msg) {
+ Ok(s) => {
+ println!("{s}");
+ 0
+ }
+ Err(_) => {
+ eprintln!("print received non-UTF8 data");
+ -1
+ }
+ }
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut msg = vec![0u8; l as usize]; | |
| let m = c.mem.as_ref().unwrap().view(&s); | |
| if m.read(p as u64, &mut msg).is_err() { | |
| return -1; | |
| } | |
| let s = std::str::from_utf8(&msg).expect("print got non-utf8 str"); | |
| println!("{s}"); | |
| 0 | |
| } | |
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut msg = vec![0u8; l as usize]; | |
| let m = c.mem.as_ref().expect("memory not initialized").view(&s); | |
| if m.read(p as u64, &mut msg).is_err() { | |
| return -1; | |
| } | |
| match std::str::from_utf8(&msg) { | |
| Ok(s) => { | |
| println!("{s}"); | |
| 0 | |
| } | |
| Err(_) => { | |
| eprintln!("print received non-UTF8 data"); | |
| -1 | |
| } | |
| } | |
| } |
Sorry, something went wrong.
| fn main() { | ||
| let mut store = Store::default(); | ||
| let module = Module::new( | ||
| &store, | ||
| &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Add command-line argument validation.
Line 91 calls nth(1).unwrap() without checking if the argument exists, which will panic if no WASM file path is provided. Consider adding proper error handling.
Apply this diff:
fn main() {
+ let wasm_path = std::env::args()
+ .nth(1)
+ .expect("Usage: wasm-runtime <wasm_file_path>");
+
let mut store = Store::default();
- let module = Module::new(
- &store,
- &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(),
- )
- .unwrap();
+ let wasm_bytes = std::fs::read(&wasm_path)
+ .expect("Failed to read WASM file");
+ let module = Module::new(&store, &wasm_bytes)
+ .expect("Failed to create WASM module");‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn main() { | |
| let mut store = Store::default(); | |
| let module = Module::new( | |
| &store, | |
| &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(), | |
| ) | |
| .unwrap(); | |
| fn main() { | |
| let wasm_path = std::env::args() | |
| .nth(1) | |
| .expect("Usage: wasm-runtime <wasm_file_path>"); | |
| let mut store = Store::default(); | |
| let wasm_bytes = std::fs::read(&wasm_path) | |
| .expect("Failed to read WASM file"); | |
| let module = Module::new(&store, &wasm_bytes) | |
| .expect("Failed to create WASM module"); |
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 87 to 93, the code calls std::env::args().nth(1).unwrap() which will panic if no argument is provided; update the main function to validate the presence of the first command-line argument, return a clear error or print a usage message and exit when it is missing, and then use the validated path when reading the file (e.g., check args().nth(1).ok_or_else(|| ...) or pattern-match and handle the None case before calling std::fs::read and Module::new).
Sorry, something went wrong.
| let inst = Instance::new(&mut store, &module, &imports).unwrap(); | ||
| env.as_mut(&mut store).mem = inst.exports.get_memory("memory").ok().cloned(); | ||
| let res = inst | ||
| .exports | ||
| .get_function("eval") | ||
| .unwrap() | ||
| .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)]) | ||
| .unwrap(); |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Replace unwrap() calls and document eval arguments.
Multiple issues:
- Lines 117, 122, 124: unwrap() calls that could panic
- Line 123: The eval function is called with (0, 0) arguments - their meaning is undocumented
Apply this diff for better error handling:
- let inst = Instance::new(&mut store, &module, &imports).unwrap();
+ let inst = Instance::new(&mut store, &module, &imports)
+ .expect("Failed to instantiate WASM module");
env.as_mut(&mut store).mem = inst.exports.get_memory("memory").ok().cloned();
- let res = inst
- .exports
- .get_function("eval")
- .unwrap()
- .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)])
- .unwrap();
+ let eval_fn = inst
+ .exports
+ .get_function("eval")
+ .expect("WASM module does not export 'eval' function");
+ // TODO: Document what these arguments (0, 0) represent
+ let res = eval_fn
+ .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)])
+ .expect("Failed to call eval function");In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 117 to 124, replace the three unwrap() calls with proper error handling and document the eval function arguments: check and return or propagate errors when creating the Instance, when getting the exported memory, and when locating/calling the "eval" function (e.g., use match/if let or ? with anyhow::Context to add descriptive messages like "failed to instantiate module", "memory export not found", "eval function not found" and include the underlying error), and add a brief comment above the call explaining what the two i32 arguments represent (for example, pointer/length or args/indexes) and derive or compute the correct argument values instead of hardcoding (0,0) or validate that (0,0) is intended before calling.
Sorry, something went wrong.
Co-Authored-By: Valentyn Faychuk <valy@faychuk.com> Co-Authored-By: Lee Dogeon <dev.moreal@gmail.com>
Fix #6281
Thanks @valentynfaychuk and @moreal !
Summary by CodeRabbit
New Features
Documentation
Chores