mirror of
https://github.com/securego/gosec.git
synced 2026-06-20 00:15:59 +02:00
fix(G118): eliminate false positive for package-level cancel variables (#1602)
* fix(G118): eliminate false positive for package-level cancel variables G118 was incorrectly reporting context cancellation function not called when the cancel function was assigned to a package-level variable (e.g., in init()) and called in a separate function (e.g., signal handler). Root cause: isCancelCalled() lacked special handling for *ssa.Global (package-level variables), causing cross-function tracking to fail. Solution: Add dedicated tracking for package-level globals, similar to the struct field handling added in PR #1596. The fix includes: - Check in *ssa.Store case to detect global variable assignments - isGlobalCalledInAnyFunc() helper to search all functions for calls - isValueCalled() generalized helper for BFS value tracking * additional test
This commit is contained in:
@@ -210,6 +210,23 @@ The following patterns are all recognised as *safe* (cancel is considered called
|
||||
| `s.cancel = cancel; defer s.cancel()` | Stored in struct field, deferred in same function |
|
||||
| `s.cancel = cancel; defer func() { s.cancel() }()` | Stored in struct field, called in closure |
|
||||
| Struct containing field is returned | Caller inherits cancel responsibility |
|
||||
| `var cancel CancelFunc` in `init()` + `cancel()` in another function | Package-level variable assigned in init, called in any function (e.g., signal handlers) |
|
||||
|
||||
Example of package-level variable pattern:
|
||||
|
||||
```go
|
||||
// Safe: cancel stored in package-level variable and called in signal handler
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
}
|
||||
|
||||
func handleShutdown() {
|
||||
cancel() // Called from signal handler
|
||||
}
|
||||
```
|
||||
|
||||
**2. Goroutine uses `context.Background`/`TODO` when request context is available (CWE-400)**
|
||||
|
||||
|
||||
@@ -719,6 +719,15 @@ func isCancelCalled(cancelValue ssa.Value, allFuncs []*ssa.Function) bool {
|
||||
return true
|
||||
}
|
||||
}
|
||||
// Check if storing to a package-level global variable.
|
||||
// When cancel is stored to a global (e.g., in init()), we need
|
||||
// to search all functions in the package for loads of that global
|
||||
// followed by a call.
|
||||
if global, ok := r.Addr.(*ssa.Global); ok {
|
||||
if isGlobalCalledInAnyFunc(global, allFuncs) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
queue = append(queue, r.Addr)
|
||||
case *ssa.UnOp:
|
||||
if r.Op == token.MUL && r.X == current {
|
||||
@@ -824,6 +833,130 @@ func isFieldCalledInAnyFunc(fa *ssa.FieldAddr, allFuncs []*ssa.Function) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// isGlobalCalledInAnyFunc checks whether a cancel function stored into a
|
||||
// package-level global variable is subsequently called in any function
|
||||
// (including init(), main(), signal handlers, etc.). This handles patterns
|
||||
// like:
|
||||
//
|
||||
// var cancel context.CancelFunc
|
||||
// func init() { _, cancel = context.WithCancel(ctx) }
|
||||
// func shutdown() { cancel() }
|
||||
func isGlobalCalledInAnyFunc(global *ssa.Global, allFuncs []*ssa.Function) bool {
|
||||
if global == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
// Iterate through all functions in the package to find loads from this global
|
||||
for _, fn := range allFuncs {
|
||||
if fn == nil || fn.Blocks == nil {
|
||||
continue
|
||||
}
|
||||
|
||||
for _, block := range fn.Blocks {
|
||||
for _, instr := range block.Instrs {
|
||||
// Look for UnOp (dereference/load) from the global
|
||||
unop, ok := instr.(*ssa.UnOp)
|
||||
if !ok || unop.Op != token.MUL {
|
||||
continue
|
||||
}
|
||||
|
||||
// Check if this load is from our global
|
||||
if unop.X != global {
|
||||
continue
|
||||
}
|
||||
|
||||
// Check if the loaded value is eventually called
|
||||
if isValueCalled(unop) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// isValueCalled checks if a value (typically a loaded function pointer) is
|
||||
// eventually used as a callee. This performs a BFS through value referrers
|
||||
// to find calls, handling phi nodes, stores/loads, type conversions, and closures.
|
||||
func isValueCalled(value ssa.Value) bool {
|
||||
if value == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
refs := value.Referrers()
|
||||
if refs == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
queue := []ssa.Value{value}
|
||||
visited := make(map[ssa.Value]bool)
|
||||
|
||||
for len(queue) > 0 {
|
||||
cur := queue[0]
|
||||
queue = queue[1:]
|
||||
|
||||
if cur == nil || visited[cur] {
|
||||
continue
|
||||
}
|
||||
visited[cur] = true
|
||||
|
||||
curRefs := cur.Referrers()
|
||||
if curRefs == nil {
|
||||
continue
|
||||
}
|
||||
|
||||
for _, ref := range *curRefs {
|
||||
switch r := ref.(type) {
|
||||
case ssa.CallInstruction:
|
||||
// Check if cur is used as the callee or an argument
|
||||
if isUsedInCall(r.Common(), cur) {
|
||||
return true
|
||||
}
|
||||
case *ssa.Phi:
|
||||
// Value flows through phi node - continue tracking
|
||||
queue = append(queue, r)
|
||||
case *ssa.Store:
|
||||
// Stored then loaded elsewhere - follow the address
|
||||
if r.Val == cur {
|
||||
queue = append(queue, r.Addr)
|
||||
}
|
||||
case *ssa.UnOp:
|
||||
// Dereference or other operation - continue tracking
|
||||
if r.X == cur {
|
||||
queue = append(queue, r)
|
||||
}
|
||||
case *ssa.ChangeType:
|
||||
// Type conversion - continue tracking
|
||||
if r.X == cur {
|
||||
queue = append(queue, r)
|
||||
}
|
||||
case *ssa.Convert:
|
||||
// Type conversion - continue tracking
|
||||
if r.X == cur {
|
||||
queue = append(queue, r)
|
||||
}
|
||||
case *ssa.MakeInterface:
|
||||
// Wrapped in interface - continue tracking
|
||||
if r.X == cur {
|
||||
queue = append(queue, r)
|
||||
}
|
||||
case *ssa.MakeClosure:
|
||||
// Captured in closure - follow into closure body
|
||||
if fn, ok := r.Fn.(*ssa.Function); ok {
|
||||
for i, binding := range r.Bindings {
|
||||
if binding == cur && i < len(fn.FreeVars) {
|
||||
queue = append(queue, fn.FreeVars[i])
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// isCancelCalledViaStructField checks whether a cancel function stored into a
|
||||
// struct field (e.g., job.cancelFn = cancel) is subsequently called in any other
|
||||
// method of the same receiver type (e.g., job.Close() calls job.cancelFn()).
|
||||
|
||||
@@ -1627,5 +1627,249 @@ func launch(ctx context.Context) {
|
||||
r.stop()
|
||||
}()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel assigned in init() and called in another function
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func shutdown() {
|
||||
cancel()
|
||||
}
|
||||
|
||||
func main() {
|
||||
shutdown()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel with signal handler pattern
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"os/signal"
|
||||
"syscall"
|
||||
)
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func handleSignal() {
|
||||
cancel()
|
||||
}
|
||||
|
||||
func main() {
|
||||
sigChan := make(chan os.Signal, 1)
|
||||
signal.Notify(sigChan, syscall.SIGTERM, syscall.SIGINT)
|
||||
go func() {
|
||||
<-sigChan
|
||||
handleSignal()
|
||||
}()
|
||||
select {}
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Vulnerable: package-level cancel never called
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func main() {
|
||||
// Never calls cancel()
|
||||
select {}
|
||||
}
|
||||
`}, 1, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel called via defer
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func cleanup() {
|
||||
defer cancel()
|
||||
}
|
||||
|
||||
func main() {
|
||||
defer cleanup()
|
||||
select {}
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel called in goroutine
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func background() {
|
||||
go func() {
|
||||
cancel()
|
||||
}()
|
||||
}
|
||||
|
||||
func main() {
|
||||
background()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Vulnerable: multiple package-level cancels, one not called
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel1, cancel2 context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx1, c1 := context.WithCancel(context.Background())
|
||||
cancel1 = c1
|
||||
_ = ctx1
|
||||
|
||||
ctx2, c2 := context.WithCancel(context.Background())
|
||||
cancel2 = c2
|
||||
_ = ctx2
|
||||
}
|
||||
|
||||
func shutdown() {
|
||||
cancel1()
|
||||
}
|
||||
|
||||
func main() {
|
||||
shutdown()
|
||||
}
|
||||
`}, 1, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel called in closure
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func setup() {
|
||||
defer func() {
|
||||
cancel()
|
||||
}()
|
||||
}
|
||||
|
||||
func main() {
|
||||
setup()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel called via method
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
type App struct{}
|
||||
|
||||
func init() {
|
||||
ctx, c := context.WithCancel(context.Background())
|
||||
cancel = c
|
||||
_ = ctx
|
||||
}
|
||||
|
||||
func (a *App) Stop() {
|
||||
cancel()
|
||||
}
|
||||
|
||||
func main() {
|
||||
app := &App{}
|
||||
defer app.Stop()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel passed as argument (tests CallInstruction with arg)
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
_, cancel = context.WithCancel(context.Background())
|
||||
}
|
||||
|
||||
func invoke(fn func()) {
|
||||
fn()
|
||||
}
|
||||
|
||||
func execute() {
|
||||
invoke(cancel)
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
|
||||
// Safe: package-level cancel in nested defer closure (tests MakeClosure)
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import "context"
|
||||
|
||||
var cancel context.CancelFunc
|
||||
|
||||
func init() {
|
||||
_, cancel = context.WithCancel(context.Background())
|
||||
}
|
||||
|
||||
func setup() {
|
||||
defer func() {
|
||||
defer cancel()
|
||||
}()
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user