Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc/wasm: wasm_exec.js not protected against growth in a few try/catch cases #45433

Closed
FrankReh opened this issue Apr 7, 2021 · 3 comments
Closed
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FrankReh
Copy link

FrankReh commented Apr 7, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes. But I'm just reading through the code in misc/wasm/wasm_exec.js. Don't have a test case that makes it fail.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

What did you expect to see?

The sp variable, representing the wasm stack pointer, at the time the imported function is being called, is being carefully reevaluated in most places where the wasm heap may have grown before using it to write results back to the stack. But there seem to be a few places that were missed; each time in the catch block of a try...catch statement.

This isn't going to be hit very often because it requires the block being tried throw an exception and the wasm memory being grown during whatever was being tried.

What did you see instead?

Here are a version of diffs I put into my own copy of wasm_exec.js without ill effect.

diff --git a/misc/wasm/wasm_exec.js b/misc/wasm/wasm_exec.js
index 82041e6bb9..8f21d5d751 100644
--- a/misc/wasm/wasm_exec.js
+++ b/misc/wasm/wasm_exec.js
@@ -401,6 +401,7 @@
     storeValue(sp + 56, result);
     this.mem.setUint8(sp + 64, 1);
 } catch (err) {
+    sp = this._inst.exports.getsp() >>> 0; // see comment above
     storeValue(sp + 56, err);
     this.mem.setUint8(sp + 64, 0);
 }
@@ -417,6 +418,7 @@
     storeValue(sp + 40, result);
     this.mem.setUint8(sp + 48, 1);
 } catch (err) {
+    sp = this._inst.exports.getsp() >>> 0; // see comment above
     storeValue(sp + 40, err);
     this.mem.setUint8(sp + 48, 0);
 }
@@ -433,6 +435,7 @@
     storeValue(sp + 40, result);
     this.mem.setUint8(sp + 48, 1);
 } catch (err) {
+    sp = this._inst.exports.getsp() >>> 0; // see comment above
     storeValue(sp + 40, err);
     this.mem.setUint8(sp + 48, 0);
 }
 
@dmitshur dmitshur added arch-wasm WebAssembly issues NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 7, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 7, 2021
@dmitshur dmitshur changed the title wasm: wasm_exec not protected against growth in a few try/catch cases misc/wasm: wasm_exec.js not protected against growth in a few try/catch cases Apr 7, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Apr 7, 2021

CC @neelance, @cherrymui via owners.

@neelance
Copy link
Member

@FrankReh Good catch! This seems correct. Mind opening a CL for it?

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 18, 2021
@gopherbot
Copy link

Change https://golang.org/cl/321936 mentions this issue: misc/wasm: ensure correct stack pointer in catch clauses

@golang golang locked and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants