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

runtime: off-by-one-error in wasmTruncS and wasmTruncU causes a crash #38839

Closed
evanw opened this issue May 3, 2020 · 3 comments
Closed

runtime: off-by-one-error in wasmTruncS and wasmTruncU causes a crash #38839

evanw opened this issue May 3, 2020 · 3 comments
Milestone

Comments

@evanw
Copy link

@evanw evanw commented May 3, 2020

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

$ go version
go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes, this is 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=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
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"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/j6/np400cw17sz0n5ljd67byrzw0000gn/T/go-build935434891=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

This is a reduced test case from a real-world issue: evanw/esbuild#83.

The contents of main.go:

package main

import (
  "fmt"
  "os"
  "strconv"
)

func main() {
  value, err := strconv.ParseFloat(os.Args[1], 64)
  if err != nil {
    panic(err)
  }
  fmt.Println(int64(value))
  fmt.Println(uint64(value))
}

Compile with this:

GOOS=js GOARCH=wasm go build -o main.wasm main.go

Run with this, where [number] is the number to test:

/usr/local/go/misc/wasm/go_js_wasm_exec main.wasm [number]

What did you expect to see?

This program should never crash.

What did you see instead?

This program crashes on the following inputs:

  • 9223372036854776000 (i.e. 0x7fff_ffff_ffff_ffff) for the cast to int64 (i.e. runtime.wasmTruncS)
  • 18446744073709552000 (i.e. 0xffff_ffff_ffff_ffff) for the cast to unt64 (i.e. runtime.wasmTruncU)

I believe the problem is that these runtime functions check their upper bounds with F64Gt (greater than) but they should check with F64Ge (greater than or equal to) instead.

The problem is that these values are outside the range where 64-bit floats can represent every integer, so 0x7fff_ffff_ffff_ffff is the same floating-point value as 0x8000_0000_0000_0000 (and many other nearby integers). Basically many integers both inside and outside the range map to that same floating-point value. It looks like all major JavaScript VMs consider that floating-point value outside the range and throw an exception.

The check for the lower bounds appears to be correct.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 3, 2020

I'd say the constant in that code (runtime.wasmTrunc{S,U}) is really the problem. The constant is written as an integer, but is converted to a float by the assembler. It is rounded up when converted to a float.

So yes, changing to >= would work. But really we should write a constant that's exactly representable in floating point, and use >.

I believe that constant is 0x7ffffffffffffc00 for signed and 0xfffffffffffff800 for unsigned. Proof:

package main

import "fmt"

func main() {
	for i := int64(1<<63 - 1); i >= 0; i-- {
		f := float64(i)
		x := int64(f)
		if x != -1<<63 {
			fmt.Printf("%x %x\n", i, x)
			break
		}
	}
	for i := uint64(1<<64 - 1); i >= 0; i-- {
		f := float64(i)
		x := uint64(f)
		if x != 1<<63 {
			fmt.Printf("%x %x\n", i, x)
			break
		}
	}
}

We should double-check the less than condition in wasmTruncS also.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 3, 2020

@randall77 randall77 added this to the Go1.15 milestone May 3, 2020
@agnivade agnivade changed the title Off-by-one-error in runtime.wasmTruncS and runtime.wasmTruncU causes a crash runtime: off-by-one-error in wasmTruncS and wasmTruncU causes a crash May 4, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 4, 2020

Change https://golang.org/cl/231837 mentions this issue: runtime: use correct truncated constants for float conversion

@gopherbot gopherbot closed this in 4daf871 May 6, 2020
xujianhai666 added a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
There is a range of numbers lower than 0x7fff_ffff_ffff_ffff which
cannot be represented by a 64 bit float. We set that to the correct
limit beyond which conversions can happen properly.

It appears that the negative bound check can indeed by correctly handled
by I64TruncF64S. But we use the same limit for consistency.

Fixes golang#38839

Change-Id: Ib783a22cb331fba7e6955459f41c67f9ceb53461
Reviewed-on: https://go-review.googlesource.com/c/go/+/231837
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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
You can’t perform that action at this time.