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, cmd/link: Go 1.17.2 causes WASM builds to throw command line too long with many environment variables #49011

Closed
evanw opened this issue Oct 15, 2021 · 12 comments

Comments

@evanw
Copy link

@evanw evanw commented Oct 15, 2021

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

$ go version
go version go1.17.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes. It's caused by the latest release.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/evan/Library/Caches/go-build"
GOENV="/Users/evan/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/evan/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/evan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/evan/dev/esbuild/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/j6/np400cw17sz0n5ljd67byrzw0000gn/T/go-build2118346093=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Here's a simple way to reproduce the issue:

$ echo 'package main;import"fmt";func main(){fmt.Println(0)}' > main.go
$ GOOS=js GOARCH=wasm go build -o main.wasm main.go
$ X=longlonglonglong && X="$X$X$X$X" && X="$X$X$X$X" && X="$X$X$X$X" && X="$X$X$X$X" node $(go env GOROOT)/misc/wasm/wasm_exec.js main.wasm

What did you expect to see?

This should print 0.

What did you see instead?

This fails with the following error:

Error: command line too long
    at global.Go.run (/usr/local/go/misc/wasm/wasm_exec.js:574:11)
    at /usr/local/go/misc/wasm/wasm_exec.js:630:14

There are at least two problems:

  1. With Go 1.17.2, it's now impossible to use Go WASM programs in situations with many environment variables. This comes up when trying to run something via GitHub Actions, for example.

  2. The error message command line too long is confusing because there are no command-line arguments. Instead the problem is that there are too many environment variables.

See also

@evanw
Copy link
Author

@evanw evanw commented Oct 15, 2021

I just discovered that this also impacts running tests for the same reason:

$ echo 'package main;import"testing";func TestFoo(*testing.T){}' > main_test.go
$ X=longlonglonglong && X="$X$X$X$X" && X="$X$X$X$X" && X="$X$X$X$X" && X="$X$X$X$X" PATH="$(go env GOROOT)/misc/wasm:$PATH" GOOS=js GOARCH=wasm go test main_test.go
Error: command line too long
    at global.Go.run (/usr/local/go/misc/wasm/wasm_exec.js:574:11)
    at /usr/local/go/misc/wasm/wasm_exec.js:630:14

Loading

@ALTree
Copy link
Member

@ALTree ALTree commented Oct 15, 2021

Loading

@ALTree ALTree added this to the Go1.18 milestone Oct 15, 2021
@ALTree ALTree changed the title Go 1.17.2 causes WASM builds to throw command line too long with many environment variables misc/wasm, cmd/link: Go 1.17.2 causes WASM builds to throw command line too long with many environment variables Oct 15, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Oct 15, 2021

Also CC @cherrymui, @neelance.

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Oct 15, 2021

Both command line arguments and environment variables are currently passed to WebAssembly by writing them to linear memory, and CL 354571 added a fixed limit of 4096 bytes for both.

The error message can certainly be improved to also mention environment variables.

I'm wondering if you can speak about how much more than 4096 bytes you're running into in your use case? Is it just a little over, or is it significantly over? Or is it unpredictable?

Loading

@evanw
Copy link
Author

@evanw evanw commented Oct 15, 2021

I'm the author of esbuild and I use GitHub Actions to run my Go WASM tests. Here's the full list of environment variables I'm seeing in my tests:

Click to expand the list of environment variables
_=/usr/bin/make
ACCEPT_EULA=Y
AGENT_TOOLSDIRECTORY=/opt/hostedtoolcache
ANDROID_HOME=/usr/local/lib/android/sdk
ANDROID_NDK_HOME=/usr/local/lib/android/sdk/ndk-bundle
ANDROID_NDK_LATEST_HOME=/usr/local/lib/android/sdk/ndk/23.0.7599858
ANDROID_NDK_ROOT=/usr/local/lib/android/sdk/ndk-bundle
ANDROID_SDK_ROOT=/usr/local/lib/android/sdk
ANT_HOME=/usr/share/ant
AZURE_EXTENSION_DIR=/opt/az/azcliextensions
BOOTSTRAP_HASKELL_NONINTERACTIVE=1
CHROME_BIN=/usr/bin/google-chrome
CHROMEWEBDRIVER=/usr/local/share/chrome_driver
CI=true
CONDA=/usr/share/miniconda
DEBIAN_FRONTEND=noninteractive
DEPLOYMENT_BASEPATH=/opt/runner
DOTNET_MULTILEVEL_LOOKUP=0
DOTNET_NOLOGO=1
DOTNET_SKIP_FIRST_TIME_EXPERIENCE=1
GECKOWEBDRIVER=/usr/local/share/gecko_driver
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTION=__run_19
GITHUB_ACTIONS=true
GITHUB_ACTOR=evanw
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_05e5de09-0c30-4b02-97e2-b15aa3c3b3ca
GITHUB_EVENT_NAME=push
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=
GITHUB_JOB=esbuild
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_05e5de09-0c30-4b02-97e2-b15aa3c3b3ca
GITHUB_REF=refs/heads/temp-go-wasm
GITHUB_REPOSITORY_OWNER=evanw
GITHUB_REPOSITORY=evanw/esbuild
GITHUB_RETENTION_DAYS=90
GITHUB_RUN_ATTEMPT=1
GITHUB_RUN_ID=1347838728
GITHUB_RUN_NUMBER=2312
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=8447ef9b4da93f3897dcb2f280d4181d5cd1f008
GITHUB_WORKFLOW=CI
GITHUB_WORKSPACE=/home/runner/work/esbuild/esbuild
GOROOT_1_14_X64=/opt/hostedtoolcache/go/1.14.15/x64
GOROOT_1_15_X64=/opt/hostedtoolcache/go/1.15.15/x64
GOROOT_1_16_X64=/opt/hostedtoolcache/go/1.16.9/x64
GOROOT_1_17_X64=/opt/hostedtoolcache/go/1.17.2/x64
GOROOT=/opt/hostedtoolcache/go/1.17.2/x64
GRAALVM_11_ROOT=/usr/local/graalvm/graalvm-ce-java11-21.2.0
GRADLE_HOME=/usr/share/gradle-7.2
HOME=/home/runner
HOMEBREW_CELLAR=/home/linuxbrew/.linuxbrew/Cellar
HOMEBREW_CLEANUP_PERIODIC_FULL_DAYS=3650
HOMEBREW_NO_AUTO_UPDATE=1
HOMEBREW_PREFIX=/home/linuxbrew/.linuxbrew
HOMEBREW_REPOSITORY=/home/linuxbrew/.linuxbrew/Homebrew
HOMEBREW_SHELLENV_PREFIX=/home/linuxbrew/.linuxbrew
ImageOS=ubuntu20
ImageVersion=20211011.1
INVOCATION_ID=e89f572507984074a8ef4d7b4677e023
JAVA_HOME_11_X64=/usr/lib/jvm/adoptopenjdk-11-hotspot-amd64
JAVA_HOME_8_X64=/usr/lib/jvm/adoptopenjdk-8-hotspot-amd64
JAVA_HOME=/usr/lib/jvm/adoptopenjdk-11-hotspot-amd64
JOURNAL_STREAM=8:22524
LANG=C.UTF-8
LEIN_HOME=/usr/local/lib/lein
LEIN_JAR=/usr/local/lib/lein/self-installs/leiningen-2.9.7-standalone.jar
MAKEFLAGS=
MAKELEVEL=1
MFLAGS=
NVM_DIR=/home/runner/.nvm
PATH=/home/runner/.deno/bin:/opt/hostedtoolcache/deno/1.15.1/x64:/opt/hostedtoolcache/node/14.18.0/x64/bin:/home/runner/go/bin:/opt/hostedtoolcache/go/1.17.2/x64/bin:/home/linuxbrew/.linuxbrew/bin:/home/linuxbrew/.linuxbrew/sbin:/home/runner/.local/bin:/opt/pipx_bin:/usr/share/rust/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
PERFLOG_LOCATION_SETTING=RUNNER_PERFLOG
PIPX_BIN_DIR=/opt/pipx_bin
PIPX_HOME=/opt/pipx
POWERSHELL_DISTRIBUTION_CHANNEL=GitHub-Actions-ubuntu20
PWD=/home/runner/work/esbuild/esbuild
RUNNER_NAME=GitHub Actions 2
RUNNER_OS=Linux
RUNNER_PERFLOG=/home/runner/perflog
RUNNER_TEMP=/home/runner/work/_temp
RUNNER_TOOL_CACHE=/opt/hostedtoolcache
RUNNER_TRACKING_ID=github_c55eb5e4-2f62-4ef4-87cb-caa86ebdb0aa
RUNNER_USER=runner
RUNNER_WORKSPACE=/home/runner/work/esbuild
SELENIUM_JAR_PATH=/usr/share/java/selenium-server-standalone.jar
SGX_AESM_ADDR=1
SHLVL=1
STATS_KEEPALIVE=false
SWIFT_PATH=/usr/share/swift/usr/bin
USER=runner
VCPKG_INSTALLATION_ROOT=/usr/local/share/vcpkg
XDG_CONFIG_HOME=/home/runner/.config

The text I pasted above comes in at 3919 bytes for 96 environment variables, so you'd need to add an extra 98*8=768 for the pointers for a total of 3919+768=4687 bytes just for environment variable data alone. So it's a little over 4096 bytes.

The maximum value of the offset variable in wasm_exec.js that I observe when running my tests is 9512, which is 5416 bytes of data. This is larger than 4687 bytes because I do have a few command-line arguments in my tests. And I'll probably need to go a little higher in the future if a future test needs another command-line argument, so it would be nice to have a little more wiggle room than exactly 5416 bytes.

Loading

@neelance
Copy link
Member

@neelance neelance commented Oct 17, 2021

I see no reason against increasing the limit from 4096 to 8192 bytes.

Later, after https://golang.org/cl/350737 landed, we can switch to the WASI interface for getting the arguments and environment which would remove the limit entirely.

Loading

mergequeue bot pushed a commit to airplanedev/cli that referenced this issue Oct 18, 2021
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/evanw/esbuild](https://togithub.com/evanw/esbuild) | require | patch | `v0.13.6` -> `v0.13.7` |

---

### Release Notes

<details>
<summary>evanw/esbuild</summary>

### [`v0.13.7`](https://togithub.com/evanw/esbuild/releases/v0.13.7)

[Compare Source](https://togithub.com/evanw/esbuild/compare/v0.13.6...v0.13.7)

-   Minify CSS alpha values correctly ([#&#8203;1682](https://togithub.com/evanw/esbuild/issues/1682))

    When esbuild uses the `rgba()` syntax for a color instead of the 8-character hex code (e.g. when `target` is set to Chrome 61 or earlier), the 0-to-255 integer alpha value must be printed as a floating-point fraction between 0 and 1. The fraction was only printed to three decimal places since that is the minimal number of decimal places required for all 256 different alpha values to be uniquely determined. However, using three decimal places does not necessarily result in the shortest result. For example, `128 / 255` is `0.5019607843137255` which is printed as `".502"` using three decimal places, but `".5"` is equivalent because `round(0.5 * 255) == 128`, so printing `".5"` would be better. With this release, esbuild will always use the minimal numeric representation for the alpha value:

    ```css
    /* Original code */
    a { color: #FF800080 }

    /* Old output (with --minify --target=chrome61) */
    a{color:rgba(255,128,0,.502)}

    /* New output (with --minify --target=chrome61) */
    a{color:rgba(255,128,0,.5)}
    ```

-   Match node's behavior for core module detection ([#&#8203;1680](https://togithub.com/evanw/esbuild/issues/1680))

    Node has a hard-coded list of core modules (e.g. `fs`) that, when required, short-circuit the module resolution algorithm and instead return the corresponding internal core module object. When you pass `--platform=node` to esbuild, esbuild also implements this short-circuiting behavior and doesn't try to bundle these import paths. This was implemented in esbuild using the existing `external` feature (e.g. essentially `--external:fs`). However, there is an edge case where esbuild's `external` feature behaved differently than node.

    Modules specified via esbuild's `external` feature also cause all sub-paths to be excluded as well, so for example `--external:foo` excludes both `foo` and `foo/bar` from the bundle. However, node's core module check is only an exact equality check, so for example `fs` is a core module and bypasses the module resolution algorithm but `fs/foo` is not a core module and causes the module resolution algorithm to search the file system.

    This behavior can be used to load a module on the file system with the same name as one of node's core modules. For example, `require('fs/')` will load the module `fs` from the file system instead of loading node's core `fs` module. With this release, esbuild will now match node's behavior in this edge case. This means the external modules that are automatically added by `--platform=node` now behave subtly differently than `--external:`, which allows code that relies on this behavior to be bundled correctly.

-   Fix WebAssembly builds on Go 1.17.2+ ([#&#8203;1684](https://togithub.com/evanw/esbuild/pull/1684))

    Go 1.17.2 introduces a change (specifically a [fix for CVE-2021-38297](https://go-review.googlesource.com/c/go/+/354591/)) that causes Go's WebAssembly bootstrap script to throw an error when it's run in situations with many environment variables. One such situation is when the bootstrap script is run inside [GitHub Actions](https://togithub.com/features/actions). This change was introduced because the bootstrap script writes a copy of the environment variables into WebAssembly memory without any bounds checking, and writing more than 4096 bytes of data ends up writing past the end of the buffer and overwriting who-knows-what. So throwing an error in this situation is an improvement. However, this breaks esbuild which previously (at least seemingly) worked fine.

    With this release, esbuild's WebAssembly bootstrap script that calls out to Go's WebAssembly bootstrap script will now delete all environment variables except for the ones that esbuild checks for, of which there are currently only four: `NO_COLOR`, `NODE_PATH`, `npm_config_user_agent`, and `WT_SESSION`. This should avoid a crash when esbuild is built using Go 1.17.2+ and should reduce the likelihood of memory corruption when esbuild is built using Go 1.17.1 or earlier. This release also updates the Go version that esbuild ships with to version 1.17.2. Note that this problem only affects the `esbuild-wasm` package. The `esbuild` package is not affected.

    See also:

    -   [golang/go#48797
    -   [golang/go#49011

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 24, 2021

Change https://golang.org/cl/358194 mentions this issue: cmd/link: increase reserved space for passing env on wasm

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Oct 25, 2021

@gopherbot Please open backport tracking issues.

This is a follow-up to the security fix included in Go 1.17.2 and 1.16.9 for issue #48797; without it some programs are unable to run.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 25, 2021

Backport issue(s) opened: #49153 (for 1.16), #49154 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359399 mentions this issue: [release-branch.go1.17] cmd/link: increase reserved space for passing env on wasm

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359400 mentions this issue: [release-branch.go1.16] cmd/link: increase reserved space for passing env on wasm

Loading

gopherbot pushed a commit that referenced this issue Oct 28, 2021
… env on wasm

On wasm, the wasm_exec.js helper passes the command line arguments and
environment variables via a reserved space in the wasm linear memory.
Increase this reserved space from 4096 to 8192 bytes so more environment
variables can fit into the limit.

Later, after https://golang.org/cl/350737 landed, we can switch to the
WASI interface for getting the arguments and environment. This would
remove the limit entirely.

Updates #49011.
Fixes #49153.

Change-Id: I48a6e952a97d33404ed692c98e9b49c5cd6b269b
Reviewed-on: https://go-review.googlesource.com/c/go/+/358194
Trust: Richard Musiol <neelance@gmail.com>
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit 252324e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/359400
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
… env on wasm

On wasm, the wasm_exec.js helper passes the command line arguments and
environment variables via a reserved space in the wasm linear memory.
Increase this reserved space from 4096 to 8192 bytes so more environment
variables can fit into the limit.

Later, after https://golang.org/cl/350737 landed, we can switch to the
WASI interface for getting the arguments and environment. This would
remove the limit entirely.

Updates #49011.
Fixes #49154.

Change-Id: I48a6e952a97d33404ed692c98e9b49c5cd6b269b
Reviewed-on: https://go-review.googlesource.com/c/go/+/358194
Trust: Richard Musiol <neelance@gmail.com>
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit 252324e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/359399
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@tliron
Copy link

@tliron tliron commented Oct 29, 2021

Suggested workaround for some cases:

Run with a clean environment, e.g.: env --ignore-environment go_js_wasm_exec ...

Loading

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
6 participants