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

Open
Wessie opened this issue Jun 4, 2018 · 10 comments
Open

cmd/vet: false positive on (possible context leak) #25720

Wessie opened this issue Jun 4, 2018 · 10 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Wessie
Copy link

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 The path to resolution is known, but the work has not been done. label Jun 4, 2018
@mvdan mvdan added this to the Go1.12 milestone Jun 4, 2018
@mvdan
Copy link
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
Copy link
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
Copy link
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
Copy link
Contributor

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
Copy link
Author

Wessie commented Jun 4, 2018

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

@josharian
Copy link
Contributor

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

@jorgex1
Copy link

jorgex1 commented Mar 11, 2020

discovered another variant that causes the same issues

type Thing struct {
  ctx context.Context
  cancel context.CancelFunc
}

func main() {
  ctx, cancel := context.WithCancel(parentCTX)
 
  // There is some unrelated code here.
  ...

  thing := Thing{
    ctx: ctx,
    cancel: cancel,
  }
}

Go vet doesn't complain from the above if the ctx, cancel is right next to the thing := Thing{} line though.

@tagpro
Copy link

tagpro commented Apr 16, 2020

Yeah, this is the case. As long as you use cancel in any form or factor, vet won't complain. I just simply assign cancel to _

ctx, cancel := context.WithCancel(parentCTX)
// vet doesn't complain if I do this
_ = cancel

// Some code here

go func() {
    // cancel on some condition
    cancel()
}()

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
@jiangsunan
Copy link

jiangsunan commented Jun 27, 2023

I also encountered the same issue. And _= cancel does not seem to help when cancel() is called inside a defer function.

The reproduction is exactly same as included in #58850 under "sophisticated example"

@timothy-king
Copy link
Contributor

@jiangsunan Here is the sophisticated example in #58850 (comment) the playground https://go.dev/play/p/LpWXSoZ5Ck8. Here is the relevant bit:

			cancel()
			ctx, cancel = context.WithCancel(parent)
			// _ = cancel // Uncomment and vet will pass.
			// use ctx

If you uncomment the line _ = cancel in the playground, this now passes vet in 1.20 whereas it fails when this is uncommented. Does this help with "And _= cancel does not seem to help"?

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) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

10 participants