NilAway does not handle explicit type conversions (false negative)

[thomasfleming-cs]

Summary

NilAway does not handle explicit type conversions (like t := (*T)(nil)) and will miss very obvious nil pointer dereferences. I have an idea for how to support these explicit type conversions, but I am not certain I've identified every code location that would need to change.

Current Behavior

NilAway has three false negatives in the following code:

package main

type Struct struct {
	Field int
}

type StructPtr *Struct

func main() {
	var explicit *Struct
	explicit = (*Struct)(nil) // explicit type conversion
	_ = explicit.Field        // PANIC, false negative

	explicitShort := (*Struct)(nil) // explicit type conversion
	_ = explicitShort.Field         // PANIC, false negative

	explicitWithoutParens := StructPtr(nil) // explicit type conversion without parentheses
	_ = explicitWithoutParens.Field         // PANIC, false negative

	var implicit *Struct
	implicit = nil     // implicit type conversion; literal `nil` is untyped and is converted to `*Struct`
	_ = implicit.Field // PANIC, true positive
}

The output of NilAway is the following:

$ nilaway ./... 
main.go:22:6: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
        -> typconv/main.go:22:6: literal `nil` accessed field `Field`

Each access of the struct's field panics with a nil pointer dereference, but NilAway only detects the nil dereference of implicit.

Expected Behavior

NilAway should report a potential nil panic for all four field accesses in the above code. In the language of NilAway, explicit type conversions should be potential producers of nilness.

Root Cause

Explicit type conversions and function calls have different semantics, but they are both represented in the AST as *ast.CallExpr. In other words, an *ast.CallExpr does not always represent a function call. When NilAway is considering whether an expression expr of type *ast.CallExpr can produce nil, it incorrectly assumes that the expression is a function call and expr.Fun is a function type (or builtin). Explicit type conversions are handled incorrectly and NilAway falsely claims that the expression cannot produce nil.

Solution

I was able to add a simple handler for type conversions in the *ast.CallExpr case of assertiontree.RootAssertionNode.ParseExprAsProducer (link). For example,

case *ast.CallExpr:
	if r.Pass().TypesInfo.Types[expr.Fun].IsType() {
		// This *ast.CallExpr is an explicit type conversion
		if util.ExprBarsNilness(r.Pass(), expr.Fun) {
			// The conversion is to a type that cannot be nil
			return nil, nil
		}

		// The conversion can be nil if and only if the converted expression can be nil.
		//
		// An explicit type conversion MUST have exactly one argument according to the
		// Go specification, so we can assume that expr.Args[0] exists for a
		// type-checked program. We could include a sanity check on the length of
		// expr.Args if we wanted to be absolutely safe, though.
		return r.ParseExprAsProducer(expr.Args[0], false)
	}

	// Now we can assume that `expr` is a function/method call
	// Rest of the case continues as normal...

(Note: the logic for determining whether the expression is a type conversion is based on Go's SSA compiler: https://cs.opensource.google/go/x/tools/+/refs/tags/v0.16.1:go/ssa/builder.go;l=636)

When I add the above code, NilAway catches all four nil panics:

main.go:22:6: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
        -> typconv/main.go:22:6: literal `nil` accessed field `Field`

main.go:18:6: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
        -> typconv/main.go:18:6: literal `nil` accessed field `Field`

main.go:15:6: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
        -> typconv/main.go:15:6: literal `nil` accessed field `Field`

main.go:12:6: error: Potential nil panic detected. Observed nil flow from source to dereference point: 
        -> typconv/main.go:12:6: literal `nil` accessed field `Field`

I am willing and able to submit a PR for this change, but I imagine there are other locations in NilAway that I have not looked at that should be updated to handle explicit type conversions. I figured that an issue was the best way to provide you all the information you should need to identify other places in the code that need to be fixed.

Please let me know if you have any further questions. Thank you for your hard work on a great analysis tool!

[sonalmahajan15]

@thomasfleming-cs , you are right, this is indeed a false negative. Thank you for the detailed description! We appreciate your hard work of debugging NilAway's complex codebase, and welcome you to submit a PR with your solution. :) If needed, we can always add follow-up PRs for covering other cases.