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

cmd/vet: "possible misuse of unsafe.Pointer" check false positive rate may be too high #41205

Open
hajimehoshi opened this issue Sep 3, 2020 · 16 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@hajimehoshi
Copy link
Member

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

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hajimehoshi/Library/Caches/go-build"
GOENV="/Users/hajimehoshi/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/hajimehoshi/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/hajimehoshi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/hajimehoshi/ebiten/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/60/khbk2xqn1c5bml1byjn89dwc0000gn/T/go-build111042416=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

mkdir tmp
cd tmp
go mod init foo
GOOS=windows go vet golang.org/x/sys/windows

What did you expect to see?

No warnings

What did you see instead?

$ GOOS=windows go vet golang.org/x/sys/windows
go: finding module for package golang.org/x/sys/windows
go: found golang.org/x/sys/windows in golang.org/x/sys v0.0.0-20200831180312-196b9ba8737a
# golang.org/x/sys/windows
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/env_windows.go:42:39: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1412:19: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1547:18: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1553:32: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1809:27: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1863:27: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3114:17: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3141:17: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3169:18: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3453:40: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3459:19: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3465:27: possible misuse of unsafe.Pointer
@gopherbot gopherbot added this to the Unreleased milestone Sep 3, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 3, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 3, 2020

/cc @alexbrainman @bradfitz per owners.

@rsc
Copy link
Contributor

rsc commented Sep 3, 2020

These are false positives.
(There is a reason this is not one of the vet checks that runs automatically during go test.)
The one in env_windows.go could be rewritten to avoid the check but is still OK as is.
The ones in zsyscall_windows.go cannot be rewritten.

I don't believe there's anything to do here.

@hajimehoshi
Copy link
Member Author

hajimehoshi commented Sep 3, 2020

go-vet's false positive happens not only on x/sys/windows but also on my library using Syscall. IIUC, as Syscall returns uintptr, it is impossible to use them as a pointer without violating the unsafe.Pointer rule (is that correct?).

Now I cannot run go-vet for my library on Windows CI due to this issue. I'd be happy if go-vet could avoid such false positives.

@dmitshur
Copy link
Contributor

dmitshur commented Sep 3, 2020

The check is "possible misuse of unsafe.Pointer", not "misuse of unsafe.Pointer". Based on Russ's comment, I understand there is no actual misuse of unsafe.Pointer in the golang.org/x/sys/windows package.

I'll retitle this to be about cmd/vet then, but I agree this may be working as intended. The presence of a "possible ..." check in vet is incompatible with running in CI systems such that any trigger fails the build.

@dmitshur dmitshur changed the title x/sys/windows: go vet warns possible pointer misuses cmd/vet: cannot be used in environments where false positives cannot be tolerated Sep 3, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 3, 2020

vet's documentation and README suggest that false positives are allowed by design:

Vet uses heuristics that do not guarantee all reports are genuine problems, but it can find errors not caught by the compilers.

Most of vet's checks are heuristic and can generate both false positives (flagging
correct programs) and false negatives (not flagging incorrect ones). The rate of
both these failures must be very small.

This means you cannot rely on default behavior of go vet in CI, where a false positive (that isn't a bug) is generally not acceptable.

@hajimehoshi Do you think there's an issue, or do you agree this is working as intended?

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 3, 2020
@hajimehoshi
Copy link
Member Author

I think there's still an issue. As we have already known these (uintptr usages of Syscall) are detected as false positives, the unsafe.Pointer rule and go-vet should be updated. Based on the vet's documentation, the rate of false positive (and negative) must be very small, right?

@dmitshur dmitshur removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 4, 2020
@dmitshur dmitshur changed the title cmd/vet: cannot be used in environments where false positives cannot be tolerated cmd/vet: "possible misuse of unsafe.Pointer" check false positive rate may be too high Sep 4, 2020
@toy80

This comment has been minimized.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2020

Note that the current very-high-confidence vet checks are already enabled during go test.
In the long term we are working toward enabling all of them, once they are high enough confidence.
This one in particular we may adjust or may just never enable.
But for CI purposes, I would suggest that you rely on "go test".

@timothy-king
Copy link
Contributor

One plausible way to address this is to add support to suppress false positives in vet at specific locations/functions. staticcheck supports this via //lint:ignore https://staticcheck.io/docs/#line-based-linter-directives (as do quite a few linters for other languages). OTOH this feature may result in worse code if folks suppress true positives.

But for CI purposes, I would suggest that you rely on "go test".

I am not sure we will want steer folks away from go vet in CI. But I also think anyone willing to run vet in CI needs to tolerate some FPs. Providing a mechanism for validating the FPs you are okay with/expect might be a nice feature.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jan 27, 2021
FiloSottile added a commit to FiloSottile/mkcert that referenced this issue Apr 26, 2022
copybara-service bot pushed a commit to google/certtostore that referenced this issue Dec 7, 2022
Update actions to v3.
Go vet is commented out due to false positives in golang/go#41205.

PiperOrigin-RevId: 492508300
copybara-service bot pushed a commit to google/certtostore that referenced this issue Dec 7, 2022
Update actions to v3.
Go vet is commented out due to false positives in golang/go#41205.

PiperOrigin-RevId: 492508300
copybara-service bot pushed a commit to google/certtostore that referenced this issue Dec 7, 2022
Update actions to v3.
Go vet is commented out due to false positives in golang/go#41205.

PiperOrigin-RevId: 492508300
copybara-service bot pushed a commit to google/certtostore that referenced this issue Dec 7, 2022
Update actions to v3.
Go vet is commented out due to false positives in golang/go#41205.

PiperOrigin-RevId: 493685245
@bcmills
Copy link
Contributor

bcmills commented Feb 3, 2023

It turns out to be possible to suppress all of these warnings in x/sys, I think. See https://go.dev/cl/465235.

@gopherbot
Copy link

Change https://go.dev/cl/465235 mentions this issue: all: silence unsafeptr vet checks

@gopherbot
Copy link

Change https://go.dev/cl/465596 mentions this issue: internal/vettest: test that all packages in x/sys are free of vet warnings

@gopherbot
Copy link

Change https://go.dev/cl/465597 mentions this issue: unix: add missing address operator in initxattrdest

@gopherbot
Copy link

Change https://go.dev/cl/465676 mentions this issue: unix: mitigate uintptr violations in PtraceIO on freebsd

gopherbot pushed a commit to golang/sys that referenced this issue Feb 6, 2023
The purpose of the _zero variable is to provide a valid address for a
pointer to an array of length zero. All other uses of the variable
take its address, but one reference to it (added in CL 147850043)
converts the variable (which has type uintptr) directly to an
unsafe.Pointer, producing a nil pointer instead of a non-nil pointer
to a zero-length array.

This typo is caught by 'go vet', but was masked for a long time by the
numerous false-positive warnings for the same check (#41205).

For golang/go#41205.

Change-Id: Ib3bebfadc6fc5574db19630169ff3f65da857bdd
Reviewed-on: https://go-review.googlesource.com/c/sys/+/465597
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit to golang/sys that referenced this issue Feb 8, 2023
In CL 419915, both pointer fields of the PtraceIoDesc struct were
converted to type uintptr to address golang/go#54113.

However, that change was overzealous: the fix needed to convert fields
that refer to addresses in the child process, but the Addr field of
PtraceIoDesc is not even in the child process! It is instead an
address in the parent (Go) process.

Go's unsafe.Pointer rules prohibit converting a Go pointer to a
uintptr except when immediately converting back to an unsafe.Pointer
or calling a system call. Populating a PtraceIoDesc struct is neither
of those things, so converting the Addr field to uintptr introduced a
use-after-free bug.

This change reverts the change to the Addr field from CL 419915 and
consolidates the implementation of PtraceIO to reduce the the amount
of code that varies with GOARCH.

This change does not address the remaining ptrace uintptr bug
(golang/go#58387), which is also present in the Linux implementation.

Fixes golang/go#58351.
Updates golang/go#54113.
For golang/go#41205.

Change-Id: I14bdb4af42130aa7b4375e3f53fd1a0435f14307
Reviewed-on: https://go-review.googlesource.com/c/sys/+/465676
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@ecsdantas
Copy link

Meanwhile the Go Team do not fix this issue, you can disable the warning by editing the settings.json in vscode.

{
   "go.useLanguageServer": true, // if this is true, needs gopls/analyses flag.
    "go.vetFlags": [
        "-unsafeptr=false"
    ],
    "gopls": {
        "analyses": { "unsafeptr": false }
    }
}

@gopherbot
Copy link

Change https://go.dev/cl/492415 mentions this issue: windows: use unsafe.Add instead of pointer arithmetic on a uintptr

gopherbot pushed a commit to golang/sys that referenced this issue May 3, 2023
The existing uintptr arithmetic is arguably valid because the
environment block is not located within the Go heap
(see golang/go#58625).

However, unsafe.Add (added in Go 1.17) expresses the same logic with
fewer conversions, and in addition avoids triggering the unsafeptr
vet check.

For golang/go#41205.

Change-Id: Ifc509279a13fd707be570908ec779d8518b4f75b
Reviewed-on: https://go-review.googlesource.com/c/sys/+/492415
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants