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

x/tools/internal/memoize: is not checkptr safe #35125

Closed
bradfitz opened this issue Oct 23, 2019 · 4 comments
Closed

x/tools/internal/memoize: is not checkptr safe #35125

bradfitz opened this issue Oct 23, 2019 · 4 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 23, 2019

This package seems to try to do "weak references":

...
        // now add the weak reference to the handle into the map                                                                                                                                                                                                                                     
        s.entries[key] = uintptr(unsafe.Pointer(h))
        // add the deletion the entry when the handle is garbage collected                                                                                                                                                                                                                           
        runtime.SetFinalizer(h, release)

....
func (s *Store) get(key interface{}) *Handle {
        // this must be called with the store mutex already held                                                                                                                                                                                                                                     
        e, found := s.entries[key]
        if !found {
                return nil
        }
        return (*Handle)(unsafe.Pointer(e))
}

But the new checkptr code is not happy with that:

https://storage.googleapis.com/go-build-log/e9e4f6c4/linux-amd64-race_3b60bfef.log

:: Running /workdir/go/bin/go with args ["/workdir/go/bin/go" "test" "-short" "-race" "golang.org/x/tools/gopls/..."] and env ["PATH=/workdir/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" "HOSTNAME=buildlet-linux-jessie-rn12a63ca" "DEBIAN_FRONTEND=noninteractive" "HOME=/root" "USER=root" "GO_STAGE0_NET_DELAY=12s" "GO_STAGE0_DL_DELAY=100ms" "WORKDIR=/workdir" "GOROOT_BOOTSTRAP=/workdir/go1.4" "GO_BUILDER_NAME=linux-amd64-race" "GOROOT_BOOTSTRAP=/go1.4" "GO_DISABLE_OUTBOUND_NETWORK=1" "GOROOT=/workdir/go" "GOPATH=/workdir/gopath" "GOPROXY=http://10.240.0.27:30157" "TMPDIR=/workdir/tmp" "GOCACHE=/workdir/gocache"] in dir /workdir/gopath/src/golang.org/x/tools/gopls

go: downloading github.com/sergi/go-diff v1.0.0
go: downloading honnef.co/go/tools v0.0.1-2019.2.3
go: extracting github.com/sergi/go-diff v1.0.0
go: extracting honnef.co/go/tools v0.0.1-2019.2.3
go: downloading github.com/BurntSushi/toml v0.3.1
go: extracting github.com/BurntSushi/toml v0.3.1
go: finding github.com/sergi/go-diff v1.0.0
go: finding honnef.co/go/tools v0.0.1-2019.2.3
go: finding github.com/BurntSushi/toml v0.3.1
?   	golang.org/x/tools/gopls	[no test files]
ok  	golang.org/x/tools/gopls/internal/hooks	1.033s
2019/10/23 22:13:15 no dep handle: no metadata for nosuchpkg
	package = nosuchpkg
2019/10/23 22:13:15 Error:2019/10/23 22:13:15 no dep handle: no metadata for nosuchpkg
	package = nosuchpkg
panic: runtime error: unsafe pointer arithmetic

goroutine 2217 [running]:
golang.org/x/tools/internal/memoize.(*Store).get(...)
	/workdir/gopath/src/golang.org/x/tools/internal/memoize/memoize.go:130
golang.org/x/tools/internal/memoize.(*Store).Bind(0xc00031d2f0, 0xfaa440, 0xc002613a70, 0xc002552360, 0x0)
	/workdir/gopath/src/golang.org/x/tools/internal/memoize/memoize.go:82 +0x477
golang.org/x/tools/internal/lsp/cache.(*cache).GetFile(0xc00031d2c0, 0xc000414000, 0x72, 0x0, 0xc000219d18, 0xc0001f2278)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/cache.go:64 +0x1ef
golang.org/x/tools/internal/lsp/cache.(*session).openOverlay(0xc0001f2160, 0x122fba0, 0xc002613a10, 0xc000414000, 0x72, 0x0, 0xc001e66120, 0x90, 0x90)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/session.go:323 +0x373
golang.org/x/tools/internal/lsp/cache.(*session).DidOpen(0xc0001f2160, 0x122fae0, 0xc00263b100, 0xc000414000, 0x72, 0x0, 0xc001e66120, 0x90, 0x90, 0x5, ...)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/session.go:244 +0x470
golang.org/x/tools/internal/lsp.(*Server).didOpen(0xc00021a120, 0x122fae0, 0xc00263b100, 0xc00263b180, 0xc0001ee160, 0xfaab40)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/text_synchronization.go:29 +0x212
golang.org/x/tools/internal/lsp.(*Server).DidOpen(0xc00021a120, 0x122fae0, 0xc00263b100, 0xc00263b180, 0xc00263b180, 0x0)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/server.go:135 +0x57
golang.org/x/tools/internal/lsp/protocol.serverHandler.Deliver(0x1246ac0, 0xc00021a120, 0x122fae0, 0xc00263b100, 0xc00263b140, 0xc0001d7600, 0x45cdca)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/protocol/tsserver.go:111 +0x2a41
golang.org/x/tools/internal/jsonrpc2.(*Conn).Run.func1(0xc0001d7620, 0xc00263b140, 0xc00021a180, 0x122fae0, 0xc00263b100, 0x0, 0x0, 0xc0010e93f0)
	/workdir/gopath/src/golang.org/x/tools/internal/jsonrpc2/jsonrpc2.go:370 +0x1c6
created by golang.org/x/tools/internal/jsonrpc2.(*Conn).Run
	/workdir/gopath/src/golang.org/x/tools/internal/jsonrpc2/jsonrpc2.go:354 +0x804
FAIL	golang.org/x/tools/gopls/test	2.544s
FAIL

/cc @mdempsky

@bradfitz bradfitz added this to the Go1.14 milestone Oct 23, 2019
@gopherbot gopherbot added the Tools label Oct 23, 2019
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Oct 23, 2019

Also #35121.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 23, 2019

That code violates package unsafe's safety rules, but it looks like it should be safe in practice. Adding a //go:nocheckptr annotation to (*Store).get seems reasonable to me.

Arguably the runtime (or at least somewhere in the standard library) should provide weak reference support. I think in the past we've discussed that weak references are redundant with finalizers; but I don't see how to implement weak references with SetFinalizer while also strictly following the current unsafe.Pointer safety rules.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 24, 2019

Change https://golang.org/cl/203078 mentions this issue: internal/memoize: add a go:nocheckptr annotation to (*Store).get

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 24, 2019

I think in the past we've discussed that weak references are redundant with finalizers; but I don't see how to implement weak references with SetFinalizer while also strictly following the current unsafe.Pointer safety rules.

Weak references require an additional layer of indirection on the consumer side.
(See #32779 (comment) for more detail.)

@golang golang locked and limited conversation to collaborators Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.