From 4befe05fc4d015806f848d7455034e652df7e696 Mon Sep 17 00:00:00 2001 From: Yash Desai Date: Wed, 14 Jan 2026 11:11:44 -0800 Subject: [PATCH] fix: track nil flows through function variables Previously, nilaway would skip nil analysis for calls made through function variables (e.g., `var ProcessFunc = process; ProcessFunc(nil)`), treating them as trusted non-nil. This caused false negatives where nil pointer dereferences were not detected. This change adds support for resolving function variables to their underlying function declarations, enabling proper nil flow tracking through both return values and arguments. Changes: - Add resolveFuncVariable() helper to resolve function variables to their underlying *types.Func for both package-level declarations (var F = f) and local assignments (f := someFunc) - Update ParseExprAsProducer() to use resolved functions for return value analysis when calling through function variables - Update AddComputation() to use resolved functions for argument consumption tracking when calling through function variables - Add getFuncReturnProducersForFunc() to support direct *types.Func usage without requiring an *ast.Ident - Add integration tests for function variable nil tracking If resolution fails (dynamic assignment, closures, etc.), the code falls back to the previous trusted behavior to avoid false positives. Fixes #387 --- .../assertiontree/parse_expr_producer.go | 20 +++- .../assertiontree/root_assertion_node.go | 61 +++++++++++ nilaway_test.go | 1 + .../go.uber.org/funcvariable/funcvariable.go | 103 ++++++++++++++++++ 4 files changed, 183 insertions(+), 2 deletions(-) create mode 100644 testdata/src/go.uber.org/funcvariable/funcvariable.go diff --git a/assertion/function/assertiontree/parse_expr_producer.go b/assertion/function/assertiontree/parse_expr_producer.go index 5d7b8282..feaf9cbc 100644 --- a/assertion/function/assertiontree/parse_expr_producer.go +++ b/assertion/function/assertiontree/parse_expr_producer.go @@ -287,9 +287,20 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool) } // Check if the method is a function value, e.g., `f := func() {}` and then `f()`. - // TODO: this is a temporary fix to suppress false positives caused by function values. - // Remove this once we have have implemented the function value support. + // For function variables, attempt to resolve to the underlying function for proper nil tracking. + // If resolution fails (dynamic assignment, etc.), fall back to trusted behavior. if r.isVariable(fun) { + // Try to resolve the function variable to its underlying function + if resolvedFunc := r.resolveFuncVariable(fun); resolvedFunc != nil { + // Successfully resolved - use the resolved function for analysis + if !doNotTrack && litArgs() { + return TrackableExpr{&funcAssertionNode{ + decl: resolvedFunc, args: expr.Args}}, nil + } + // function call has non-literal args, so is not literal, use its return annotation + return nil, r.getFuncReturnProducersForFunc(resolvedFunc, expr) + } + // Could not resolve - fall back to trusted behavior to avoid false positives return nil, []producer.ParsedProducer{producer.ShallowParsedProducer{Producer: &annotation.ProduceTrigger{ Annotation: &annotation.TrustedFuncNonnil{ProduceTriggerNever: &annotation.ProduceTriggerNever{}}, Expr: expr, @@ -533,7 +544,12 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool) // getFuncReturnProducers returns a list of producers that are triggered at the call expression func (r *RootAssertionNode) getFuncReturnProducers(ident *ast.Ident, expr *ast.CallExpr) []producer.ParsedProducer { funcObj := r.ObjectOf(ident).(*types.Func) + return r.getFuncReturnProducersForFunc(funcObj, expr) +} +// getFuncReturnProducersForFunc returns a list of producers for a given function object and call expression. +// This is the core implementation used by getFuncReturnProducers and also for resolved function variables. +func (r *RootAssertionNode) getFuncReturnProducersForFunc(funcObj *types.Func, expr *ast.CallExpr) []producer.ParsedProducer { numResults := util.FuncNumResults(funcObj) isErrReturning := util.FuncIsErrReturning(funcObj.Signature()) isOkReturning := util.FuncIsOkReturning(funcObj.Signature()) diff --git a/assertion/function/assertiontree/root_assertion_node.go b/assertion/function/assertiontree/root_assertion_node.go index 2fb5420c..7843b516 100644 --- a/assertion/function/assertiontree/root_assertion_node.go +++ b/assertion/function/assertiontree/root_assertion_node.go @@ -747,6 +747,15 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) { // Add Consumptions for struct field params r.addConsumptionsForArgAndReceiverFields(expr, fun) } + } else if fun := getFuncIdent(expr, &r.functionContext); fun != nil && r.isVariable(fun) { + // Check if this is a function variable that can be resolved to an underlying function. + // This enables nil tracking through function variables like `var ProcessFunc = process`. + if resolvedFunc := r.resolveFuncVariable(fun); resolvedFunc != nil { + consumeArg = consumeArgTrigger(resolvedFunc) + } else { + // Could not resolve the function variable - fall back to no-op + consumeArg = consumeArgNoop + } } else { // here we have found either a builtin function like make or new, // or a typecast like int(x) - in either case (at least for now), do nothing to try @@ -941,6 +950,58 @@ func getFuncLitFromAssignment(ident *ast.Ident) *ast.FuncLit { return nil } +// resolveFuncVariable attempts to resolve a function variable to the underlying function it points to. +// This handles the common pattern of package-level function variables like `var ProcessFunc = process` +// or local assignments like `f := someFunc`. Returns the resolved *types.Func if successful, nil otherwise. +// This enables nilaway to track nil flows through function variables instead of treating them as trusted. +func (r *RootAssertionNode) resolveFuncVariable(ident *ast.Ident) *types.Func { + if ident == nil || ident.Obj == nil || ident.Obj.Decl == nil { + return nil + } + + // Case 1: Package-level variable declaration (var ProcessFunc = process) + if valSpec, ok := ident.Obj.Decl.(*ast.ValueSpec); ok { + // Only handle 1-1 assignments for now + if len(valSpec.Names) != len(valSpec.Values) { + return nil + } + + for i, name := range valSpec.Names { + if name.Obj != ident.Obj { + continue + } + // Check if RHS is an identifier pointing to a function + if rhsIdent, ok := valSpec.Values[i].(*ast.Ident); ok { + if funcObj, ok := r.Pass().TypesInfo.ObjectOf(rhsIdent).(*types.Func); ok { + return funcObj + } + } + } + } + + // Case 2: Local assignment statement (f := someFunc or f = someFunc) + if assign, ok := ident.Obj.Decl.(*ast.AssignStmt); ok { + if len(assign.Lhs) != len(assign.Rhs) { + return nil + } + + for i := range assign.Lhs { + lhsIdent, ok := assign.Lhs[i].(*ast.Ident) + if !ok || lhsIdent.Obj != ident.Obj { + continue + } + // Check if RHS is an identifier pointing to a function + if rhsIdent, ok := assign.Rhs[i].(*ast.Ident); ok { + if funcObj, ok := r.Pass().TypesInfo.ObjectOf(rhsIdent).(*types.Func); ok { + return funcObj + } + } + } + } + + return nil +} + // LiftFromPath takes a `path` of assertion nodes, and searches for it in the assertion tree rooted // at `rootNode`. If found, it removes that tree and returns its root as `node`, with `ok` = true. // If not found, it returns `node`, `ok` = nil, false diff --git a/nilaway_test.go b/nilaway_test.go index 63d3fd7e..dc6a3ed6 100644 --- a/nilaway_test.go +++ b/nilaway_test.go @@ -75,6 +75,7 @@ func TestNilAway(t *testing.T) { {name: "AbnormalFlow", patterns: []string{"go.uber.org/abnormalflow"}}, {name: "NoLint", patterns: []string{"go.uber.org/nolint"}}, {name: "Templ", patterns: []string{"go.uber.org/templ"}}, + {name: "FuncVariable", patterns: []string{"go.uber.org/funcvariable"}}, } for _, tt := range tests { diff --git a/testdata/src/go.uber.org/funcvariable/funcvariable.go b/testdata/src/go.uber.org/funcvariable/funcvariable.go new file mode 100644 index 00000000..701672d4 --- /dev/null +++ b/testdata/src/go.uber.org/funcvariable/funcvariable.go @@ -0,0 +1,103 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/* +These tests check nil tracking through function variables. + + +*/ +package funcvariable + +type Config struct { + Value string +} + +// nilable(result 0) +func nilableFunc() *Config { + return nil +} + +func nonNilFunc() *Config { + return &Config{Value: "test"} +} + +// Package-level function variable pointing to a nilable function +var NilableFuncVar = nilableFunc + +// Package-level function variable pointing to a non-nil function +var NonNilFuncVar = nonNilFunc + +// Test 1: Direct call to nilable function - should be detected +func testDirectNilableCall() { + cfg := nilableFunc() + _ = cfg.Value //want "accessed field" +} + +// Test 2: Call through function variable pointing to nilable function - should be detected +func testFuncVarNilableCall() { + cfg := NilableFuncVar() + _ = cfg.Value //want "accessed field" +} + +// Test 3: Direct call to non-nil function - should NOT be detected +func testDirectNonNilCall() { + cfg := nonNilFunc() + _ = cfg.Value // OK - nonNilFunc always returns non-nil +} + +// Test 4: Call through function variable pointing to non-nil function - should NOT be detected +func testFuncVarNonNilCall() { + cfg := NonNilFuncVar() + _ = cfg.Value // OK - NonNilFuncVar points to nonNilFunc which always returns non-nil +} + +// Test 5: Local function variable assigned from nilable function +func testLocalFuncVarNilable() { + f := nilableFunc + cfg := f() + _ = cfg.Value //want "accessed field" +} + +// Test 6: Local function variable assigned from non-nil function +func testLocalFuncVarNonNil() { + f := nonNilFunc + cfg := f() + _ = cfg.Value // OK - f points to nonNilFunc +} + +// Test 7: Function that takes a pointer and dereferences it +func process(cfg *Config) { + // The nil flow is detected at the call site, not here + println(cfg.Value) +} + +// Test 8: Package-level function variable for process +var ProcessFunc = process + +// Test 9: Calling process through function variable with nil - should be detected +func testProcessFuncVarNil() { + ProcessFunc(nil) //want "passed" +} + +// Test 10: Calling process directly with nil - should be detected +func testProcessDirectNil() { + process(nil) //want "passed" +} + +// Test 11: Calling process with non-nil - should NOT be detected +func testProcessNonNil() { + cfg := &Config{Value: "test"} + process(cfg) // OK + ProcessFunc(cfg) // OK +}