golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.59k stars 17.48k forks source link

syscall/js: does not handle aliased pointers (panics & incorect results) #60462

Open Jorropo opened 1 year ago

Jorropo commented 1 year ago

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

$ go version
go version go1.20.4 linux/amd64

Does this issue reproduce with the latest release?

Yes (go1.20.4)

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

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

What did you do?

$ cd /tmp && git clone https://github.com/Jorropo/go-bug-report-syscall-js-reference-cycle && cd go-bug-report-syscall-js-reference-cycle
$ ./serve.sh
$ firefox http://localhost:8081/

What did you expect to see?

Capture d’écran du 2023-05-26 17-40-49

What did you see instead?

Capture d’écran du 2023-05-26 17-40-04

Jorropo commented 1 year ago

This came up in arguably dubious real world code of mine however I think this could be a realistical usecase in some complex applications. Fixing this wouldn't be very hard (maintain a map of all pointers converted and reuse items of the map instead of recursing until infinity). Imo the best fix is to go with #60335 and remove support for any recursion based conversion in ValueOf but this is breaking the Go1 compatibility.

Jorropo commented 1 year ago
package main

import "syscall/js"

func main() {
    a := []any{nil}
    b := []any{a, a}
    js.Global().Call("foo", js.ValueOf(b))
}

Also provides an incorrect result here it makes two copies of a in JS land instead of one copy with two aliased pointers, available at git clone -b other-manifestation https://github.com/Jorropo/go-bug-report-syscall-js-reference-cycle.

Jorropo commented 1 year ago

I'll submit a fix for this later over the week.

Jorropo commented 1 year ago

Fixing this wouldn't be very hard (maintain a map of all pointers converted and reuse items of the map instead of recursing until infinity).

This is way harder than it looks, the issue is slices, consider thoses case:

a := []int{1,2,3,4,5}
b := a[2:]
return js.ValueOf([]any{a[:4:4], b})
return js.ValueOf([]any{b, a})

A simple recursive descent which memoize pointers into a map would first create a partial copy of the slice, before later then finding they need to be aliased. So either you maintain a list of potentially aliased slices and go update everytime theses kinds of clashes happen. Or you do a runtime pointer analysis first and then do a second scan this time producing the slices correctly.

This could also be solved by intrinsifying js.ValueOf and trying to solve the aliasing problem at compile time, my guess is that this almost never comes up, and a conservative compiler that solves the easy cases of pointer aliasing would be able to solve this. This is clearly a nuclear option here.