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, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

Open
dmitshur opened this issue Mar 15, 2021 · 7 comments · May be fixed by orijtech/sigchanyzer#8
Assignees
Labels
Milestone

Comments

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 15, 2021

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

$ gotip version
go version devel +8ed438c07 Mon Mar 15 20:48:37 2021 +0000 darwin/amd64

Does this issue reproduce with the latest release?

No.

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

go env Output
$ gotip env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/dmitshur/Library/Caches/go-build"
GOENV="/Users/dmitshur/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/dmitshur/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/dmitshur/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/dmitshur/sdk/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/dmitshur/sdk/gotip/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel +8ed438c07 Mon Mar 15 20:48:37 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/tmp.YEX3UfOo/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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/go-build3265915026=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I wrote a program that appears to be correct. It uses signal.Notify to catch SIGINT signals to prevent default handling but otherwise completely ignore them:

package main

import (
	"os"
	"os/signal"
	"time"
)

func main() {
	signal.Notify(make(chan os.Signal), os.Interrupt) // ignore SIGINT
	time.Sleep(10 * time.Second)
}

Then ran gotip vet . on it.

What did you expect to see?

No vet report, because as far as I can tell the os.Signal channel doesn't need to be buffered when it's completely unused.

What did you see instead?

$ gotip vet .
# m.test
./main.go:10:2: misuse of unbuffered os.Signal channel as argument to signal.Notify

CC @odeke-em, @ianlancetaylor, @timothy-king.

@dmitshur dmitshur added this to the Backlog milestone Mar 15, 2021
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 15, 2021

I am not sure I understand why this is safe yet. (So no informed cmd/vet opinion yet.)

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 15, 2021

This came up in CL 217765 for #36976, where I copied a function from cmd/go/internal/base to the dl wrapper to make sure the parent process doesn't also spit out traces. But, cmd/go needs to actually use the signal to make a note that it was interrupted. dl doesn't, so doesn't actually care whether or not it can observe the signal (it's just going to exit once the child is gone anyway).

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 16, 2021

Code like signal.Notify(make(chan os.Signal), os.Interrupt) is safe because the runtime always writes to the channel using a non-blocking send. This is in effect a way to ignore the signal. (It would be better to use signal.Ignore, but that is a separate issue.)

It's not safe to write code like

    c := make(chan os.Signal)
    signal.Notify(c, os.Interrupt)
    // use c somewhere

because the fact that the runtime uses a non-blocking send means that if the channel is unbuffered signals can be lost. In the case where the make is invoked directly in the call to signal.Notify, nothing is ever going to read from the channel, so the fact that signals can be lost is unimportant.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 16, 2021

Roger that, thanks everyone for the discussion. I shall send a change tomorrow morning/afternoon, and get this fixed.

@odeke-em odeke-em self-assigned this Mar 16, 2021
@odeke-em odeke-em modified the milestones: Backlog, Go1.17 Mar 16, 2021
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 16, 2021

Thanks for the pointer to signal.Ignore, I didn't remember that there was a distinct function for this. (I don't think I can use it in my CL anyway, given I assume golang.org/dl probably still needs to support Go 1.4. Oh well.)

@AGMETEOR
Copy link

@AGMETEOR AGMETEOR commented Mar 23, 2021

@odeke-em , I would like to fix this in the sigchanyzer

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 23, 2021

All yours, thank you @AGMETEOR and welcome to the Go project, great to have you become a contributor!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants