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

proposal: cmd/go: consider adding unsafeptr to go vet during test list #57794

Closed
lasiar opened this issue Jan 13, 2023 · 6 comments
Closed

proposal: cmd/go: consider adding unsafeptr to go vet during test list #57794

lasiar opened this issue Jan 13, 2023 · 6 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@lasiar
Copy link

lasiar commented Jan 13, 2023

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

$ go version
go version go1.19.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPRIVATE="gitlab.corp.mail.ru/taxmonitor/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/49/y7d47mwn7rx69725q6y7ylx00000gp/T/go-build1390474754=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

compile with flag -race
https://go.dev/play/p/wNXEyU8p6XD?v=gotip

What did you expect to see?

Two same addresses, example:
0xc00012e280
0xc00012e280

What did you see instead?

fatal error: checkptr: pointer arithmetic result points to invalid allocation

I think the error cause incorrect compile on this place, because original value is null.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 13, 2023
@ianlancetaylor
Copy link
Contributor

The expression lp = unsafe.Pointer(sh.Data + ui*size) is not permitted by the rules at https://pkg.go.dev/unsafe#Pointer.

@bcmills
Copy link
Contributor

bcmills commented Jan 17, 2023

Note that go vet explicitly flags this misuse:

$ go vet .
# example
./main.go:28:7: possible misuse of unsafe.Pointer

I am not sure why the unsafeptr check is not included in the default set run by go test. (It isn't mentioned in #18085.)

@bcmills bcmills changed the title runtime: checkptr false negative result. cmd/go: include checkptr in go test vet checks, or document why it is not included Jan 17, 2023
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go and removed Documentation compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 17, 2023
@bcmills bcmills added this to the Backlog milestone Jan 17, 2023
@rsc rsc changed the title cmd/go: include checkptr in go test vet checks, or document why it is not included cmd/go: consider adding unsafeptr to go vet during test list Jan 17, 2023
@rsc
Copy link
Contributor

rsc commented Jan 17, 2023

This looks like a shortcoming in the unsafe package docs that propagated into both the checkptr compiler code and the unsafeptr vet code. The compiler knows that when you read a SliceHeader or StructHeader Data field, it is as though you are writing uintptr(an unsafe.Pointer-typed field), but the docs and the checks don't apply this rule.

I don't think it's particularly defensible to say that

vp := unsafe.Pointer(sh.Data)
l1 = unsafe.Pointer(uintptr(vp) + ui*size)

is permitted but somehow the single-line form

lp = unsafe.Pointer(sh.Data + ui*size)

is not.

@mdempsky
Copy link
Member

mdempsky commented Feb 3, 2023

The compiler knows that when you read a SliceHeader or StructHeader Data field, it is as though you are writing uintptr(an unsafe.Pointer-typed field)

Caveat: The compiler knows this well enough to satisfy safety rule 6 in isolation, but I'm not certain it already satisfies it in combination with rule 3. That is, off hand, I can't think of any reason why unsafe.Pointer(sh.Data + E) (for arbitrary uintptr-typed expression E) wouldn't work today, but I've at least not had that in mind as an explicit goal when reviewing compiler code.

It's certainly not safe today to combine rules 3 and 5; e.g., unsafe.Pointer(v.Pointer() + E) can fail if E requires any function calls (including implicit runtime calls, possibly from instrumentation), because the v.Pointer() result will be spilled as a uintptr-typed temporary. We specially recognize the expression unsafe.Pointer(F(...)) (for arbitrary uintptr-returning F), and ensure that whole expression is spilled as an unsafe.Pointer-typed temporary instead.

One nice thing about that is it's safe to do regardless of what F actually is. But if we need to support unsafe.Pointer(v.Pointer() + E), then we're going to need to be stricter about recognizing just reflect.Value.Pointer and reflect.Value.UnsafeAddr calls, which then opens up the same questions as #34684 about whether indirect calls are allowed.

@rsc rsc changed the title cmd/go: consider adding unsafeptr to go vet during test list proposal: cmd/go: consider adding unsafeptr to go vet during test list Jun 28, 2023
@rsc
Copy link
Contributor

rsc commented Jun 28, 2023

I think we can close this as a duplicate of #58625. In particular, the reports examined in #58625 (comment) make clear that the unsafeptr check is not accurate enough to turn on by default in 'go test' and instead probably should be removed from vet entirely. In any event, that conversation is happening on #58625 and need not be duplicated here.

@rsc
Copy link
Contributor

rsc commented Jun 28, 2023

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group

@rsc rsc closed this as completed Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Declined
Development

No branches or pull requests

6 participants