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: Go's WASM shim breaks in Node v19 #56860

Closed
evanw opened this issue Nov 20, 2022 · 9 comments
Closed

misc/wasm: Go's WASM shim breaks in Node v19 #56860

evanw opened this issue Nov 20, 2022 · 9 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-JS
Milestone

Comments

@evanw
Copy link

evanw commented Nov 20, 2022

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

$ go version
go version go1.19.1 linux/amd64

Does this issue reproduce with the latest release?

Presumably, because the problematic code is still there in the latest commit.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/evan/.cache/go-build"
GOENV="/home/evan/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/evan/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/evan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/evan/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/evan/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4187052113=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Install node v19 from here: https://nodejs.org/dist/v19.1.0/
  2. Make a simple hello world go program
  3. Built it with GOOS=js GOARCH=wasm go build main.go
  4. Run it with node $(go env GOPATH)/misc/wasm/wasm_exec_node.js main

What did you expect to see?

Hello world

What did you see instead?

/home/evan/go/misc/wasm/wasm_exec_node.js:25
globalThis.crypto = {
                  ^

TypeError: Cannot set property crypto of #<Object> which has only a getter
    at Object.<anonymous> (/home/evan/go/misc/wasm/wasm_exec_node.js:25:19)
    at Module._compile (node:internal/modules/cjs/loader:1205:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1259:10)
    at Module.load (node:internal/modules/cjs/loader:1068:32)
    at Module._load (node:internal/modules/cjs/loader:909:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:82:12)
    at node:internal/main/run_main_module:23:47

Node.js v19.1.0

I assume this recent Node change is the cause: nodejs/node#44897. Here's one possible fix:

diff --git a/wasm_exec_node.js b/wasm_exec_node.js
index f9200ca..2b424a2 100644
--- a/wasm_exec_node.js
+++ b/wasm_exec_node.js
@@ -22,11 +22,13 @@ globalThis.performance = {
 };
 
 const crypto = require("crypto");
-globalThis.crypto = {
-       getRandomValues(b) {
-               crypto.randomFillSync(b);
+Object.defineProperty(globalThis, 'crypto', {
+       value: {
+               getRandomValues(b) {
+                       crypto.randomFillSync(b);
+               },
        },
-};
+});
 
 require("./wasm_exec");

For further context, this bug was reported to me by a user of esbuild here: evanw/esbuild#2683

@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-wasm WebAssembly issues OS-JS labels Nov 20, 2022
@seankhliao
Copy link
Member

cc @golang/wasm

@neelance
Copy link
Member

I think we can simply drop these shims because the latest Node.js v18 LTS already has them built-in. Our support policy is only to support the active LTS version.

@neelance
Copy link
Member

Oops, not entirely true. Node.js v18 has crypto.getRandomValues, but it does not load crypto by default. So we still need:

globalThis.crypto ??= require('crypto');

@johanbrandhorst
Copy link
Member

johanbrandhorst commented Dec 1, 2022

Using the suggested fix would require upgrading our js-wasm builders to LTS 18, so I've opened #57017 to track this effort. Assuming we can get that done I have confirmed locally that the suggested fix works (for both 18 and 19).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/455415 mentions this issue: misc/wasm: migrate support to Node 18 LTS

@seankhliao seankhliao added this to the Go1.21 milestone Dec 8, 2022
@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 Dec 30, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Jan 4, 2023

As I understand, the fix suggested in #56860 (comment) (and currently implemented in CL 455415) intentionally drops compatibility with Node 14 ('??=' is too new of a syntax for it; see log). That's fine to do if desired, but submitting that CL will require us to turn off the TryBot part of the current js-wasm builder with Node 14.

Note that the new js-wasm-node18 builder can't be made a TryBot until all.bash is passing on it. If that will take some investigation work (see #57614), maybe it'd be worth it to have all that ready first, to keep a small gap in TryBot coverage during the time when neither js-wasm nor js-wasm-node18 builder is passing.

Another possible path is to maintain compatibility with Node 18 and 14 only during the transition, and then drop anything only needed for Node 14 afterwards.

I think whichever path is easier for the CL author(s) is fine.

@johanbrandhorst
Copy link
Member

In my testing this fix also drops compatibility for Node 16, the ??= syntax is only available in 17+, as noted in the CL commit message. I think we wait with this fix until we have the node18 builders available and the node14 builders retired.

@merceyz
Copy link

merceyz commented Jan 4, 2023

the ??= syntax is only available in 17+

It's supported in Node.js v15.0.0 and up https://node.green/#ES2021, I'm assuming the commit is referring to https://nodejs.org/docs/latest-v19.x/api/crypto.html#cryptogetrandomvaluestypedarray which was added in v17.4.0.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/463975 mentions this issue: misc/wasm: use NodeJS crypto library

johanbrandhorst added a commit to Pryz/go that referenced this issue Feb 12, 2023
The move to NodeJS 18 allows us to replace the custom
crypto functions with the expanded crypto primitives of
the NodeJS crypto library.

Fixes golang#56860

Change-Id: I8726b4003150f31521f246f613b6976641b9fa69
Reviewed-on: https://go-review.googlesource.com/c/go/+/463975
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Evan Phoenix <evan@phx.io>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@golang golang locked and limited conversation to collaborators Jan 30, 2024
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. OS-JS
Projects
None yet
Development

No branches or pull requests

7 participants