Skip to content

cmd/vet: initialize an external context is regarded as context leak? #31856

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.

Sign up for GitHub

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
Closed
SilverRainZ opened this issue May 6, 2019 · 6 comments
Closed

cmd/vet: initialize an external context is regarded as context leak? #31856

SilverRainZ opened this issue May 6, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@SilverRainZ
Copy link
Contributor

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

$ go version
go version go1.11.5 linux/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
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/la/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/la/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/la/git/tools/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build166721615=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Variables context, cancel are declared at toplevel, function Foo is used to to initialize them,
Indeed this code is quite poor, but is it really fine to be regard as an error?

https://play.golang.org/p/fHQ_fEu0GLW

What did you expect to see?

go vet reports nothing.

What did you see instead?

go vet reports lostcancel error:

/home/la/git/tools/main.go:11:2: the cancel function is not used on all paths (possible context leak)
/home/la/git/tools/main.go:12:1: this return statement may be reached without using the cancel var defined on line 11
@SilverRainZ SilverRainZ changed the title cmd/vet: initialize an external context is regared as context leak? cmd/vet: initialize an external context is regarded as context leak? May 6, 2019
@josharian
Copy link
Contributor

cc @alandonovan @mvdan

@alandonovan
Copy link
Contributor

Seems like a bug in the analyzer. If the cancel variable has package scope (x.Parent() == x.Pkg().Scope()) it should suppress the finding.

@SilverRainZ SilverRainZ mentioned this issue May 7, 2019
Closed
@SilverRainZ
Copy link
Contributor Author

@alandonovan (x.Parent() == x.Pkg().Scope()) is just a special case for the aboved case. In my opinion, the fundamental reason is that lostcancel should skip the analysis for cancel which defined outside current function because the analysis is limited to function scope, what do you think?

This case also complained by go vet, but looks more reasonable:

// Test for Go issue 31856.
func _() {
	var cancel func()

	func() {
		_, cancel = context.WithCancel(bg)
	}()

	cancel()
}

I have filed a PR, should fix this issue.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/175617 mentions this issue: lostcancel: do not analyze cancel variable which defined outside current function scope

gopherbot pushed a commit to golang/tools that referenced this issue May 8, 2019
@SilverRainZ @adonovan
lostcancel: do not analyze cancel variable which defined outside curr…
9529901 
…ent function scope

See golang/go#31856.

Change-Id: I229a7f4a48e7806df62941f801302b6da8a0c12b
GitHub-Last-Rev: 33f8523
GitHub-Pull-Request: #95
Reviewed-on: https://go-review.googlesource.com/c/tools/+/175617
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 8, 2019
@andybons andybons added this to the Unplanned milestone May 8, 2019
@SilverRainZ
Copy link
Contributor Author

@andybons The related PR is merged, this issue shouldn't be labled as NeedsInvestigation and may can be closed.

@agnivade
Copy link
Contributor

In future, you can write "Fixes golang/go#xxxx" if you want CLs to automatically close issues when they are merged.

@golang golang locked and limited conversation to collaborators May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@josharian @andybons @agnivade @SilverRainZ @alandonovan @gopherbot