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: false positive on (possible context leak) #25720

Open
Wessie opened this Issue Jun 4, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@Wessie

Wessie commented Jun 4, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10 windows/amd64

Does this issue reproduce with the latest release?

yes, tested on go1.10.2 as well

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\micro\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=F:\goenv\
set GORACE=
set GOROOT=F:\go
set GOTMPDIR=
set GOTOOLDIR=F:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\micro\AppData\Local\Temp\go-build748724598=/tmp/go-build -gno-record-gcc-switches

What did you do?

run go vet on https://play.golang.org/p/X_vRJbYf7rO

What did you expect to see?

no errors

What did you see instead?

issue.go:12: the cancel function is not used on all paths (possible context leak)
issue.go:14: this return statement may be reached without using the cancel var defined on line 12

@mvdan mvdan added the NeedsFix label Jun 4, 2018

@mvdan mvdan added this to the Go1.12 milestone Jun 4, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Jun 4, 2018

Thanks for the report and the code to reproduce the issue. It indeed looks like vet's analysis isn't sophisticated enough. It already builds a control flow graph, so hopefully the fix won't be too complex.

@mvdan

This comment has been minimized.

Member

mvdan commented Jun 4, 2018

Here's a smaller repro:

package p

import "context"

func f(ctx context.Context) {
        var cancel func()
        defer func() { cancel() }()
        ctx, cancel = context.WithCancel(ctx)
}

If the defer is moved below the WithCancel line, vet works as expected. The issue here is that the lostcancel check never notices the cancel() call as a use right before the return (since that's how defers work).

It seems to me like this is simply that vet doesn't handle defers properly. The internal/cfg package doesn't treat them especially, and the check itself does nothing special either.

@alandonovan do you think this would be a cfg or a vet fix? For example, when building the cfg, we could move the deferred blocks to be right before the appropriate return statements. But I'm not sure if that would be outside the scope of what a control flow graph should be.

@mvdan

This comment has been minimized.

Member

mvdan commented Jun 4, 2018

It just dawned on me that the cfg builder can't possibly do what I described above. For example, if we have:

if x {
    defer call()
}
return y

The CFG will end up with a single return statement, and statically it's impossible to know if there will be a defer or not. So it seems to me like the fix must be after the CFG has been built.

@alandonovan

This comment has been minimized.

Contributor

alandonovan commented Jun 4, 2018

The error message is slightly confusing, but I think the real problem here is that you're deferring a function that uses an uninitialized variable. Admittedly WithCancel is not supposed to panic, but if it did, the deferred function would call a nil function. The right place for a cleanup operation is after the variable has been successfully initialized.

I'm not convinced it is worth complicating the checker logic to handle this case.

@Wessie

This comment has been minimized.

Wessie commented Jun 4, 2018

The original snippet does not have any uninitialized variables, and still produces the error

@josharian

This comment has been minimized.

Contributor

josharian commented Jun 4, 2018

@Wessie yes, but along the same lines, the cancel function that gets called isn’t guaranteed to be the intended one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment