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

Comments

@FrankReh
Copy link

@FrankReh 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 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 dmitshur commented Apr 7, 2021

@neelance
Copy link
Member

@neelance neelance commented Apr 18, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented May 23, 2021

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

@gopherbot gopherbot closed this in 08a8fa9 May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants