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

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

Open
Jorropo opened this issue May 26, 2023 · 4 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS

Comments

@Jorropo
Copy link
Member

Jorropo commented May 26, 2023

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
Copy link
Member Author

Jorropo commented May 26, 2023

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
Copy link
Member Author

Jorropo commented May 26, 2023

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
Copy link
Member Author

Jorropo commented May 26, 2023

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

@Jorropo Jorropo changed the title syscall/js: does not support reference cycles syscall/js: does not handle aliased pointers (panics & incorect results) May 26, 2023
@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS labels May 29, 2023
@Jorropo
Copy link
Member Author

Jorropo commented Jul 19, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS
Projects
None yet
Development

No branches or pull requests

2 participants