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

Jump to bottom

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
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@evanw
Copy link

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

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

@neelance

@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
@agnivade agnivade added arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. labels May 4, 2020
@gopherbot
Copy link

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

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@agnivade @xujianhai666
runtime: use correct truncated constants for float conversion
8585723 
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>
@golang golang locked and limited conversation to collaborators May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
4 participants
@evanw @agnivade @randall77 @gopherbot