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

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Go1.14

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Oct 23, 2019 •
edited

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 NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) labels Oct 23, 2019
@bradfitz bradfitz added this to the Go1.14 milestone Oct 23, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 23, 2019
@zikaeroh
Copy link
Contributor

Also #35121.

@mdempsky
Copy link
Member

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

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

@bcmills
Copy link
Contributor

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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
6 participants
@bradfitz @mdempsky @ianthehat @bcmills @gopherbot @zikaeroh