nilaway icon indicating copy to clipboard operation
nilaway copied to clipboard

Incomplete Nil Pointer Detection in Struct Fields

Open Yu-Qi opened this issue 1 year ago • 2 comments

Description I encountered an issue with potential nil pointer dereference in the Project struct of my Go code. I'm using the nilaway tool to detect such issues, but it seems to miss this particular case.

Code Sample:

type Project struct {
    ProjectID   *int    `json:"project_id"`
    ProjectName *string `json:"project_name"`
}

func main() {
    project := Project{}
    print("ProjectID:", *project.ProjectID)
    print("ProjectName:", *project.ProjectName)
}

Expected Behavior: As per my understanding, when the Project struct is initialized without explicitly assigning values to ProjectID and ProjectName fields, they should be set to nil.

Actual Behavior: However, when running the code, I encounter a nil pointer dereference error. It appears that nilaway is not able to detect this potential issue.

Steps to Reproduce:

  1. Create a Go program with the Project struct and similar code as provided above.
  2. Use the nilaway tool or other relevant method to check for nil pointer dereference issues.

Environment:

  • Go version: v1.20

Please let me know if there's any additional information needed or if there are any suggested solutions to address this issue. Thank you!

Yu-Qi avatar Aug 03 '23 15:08 Yu-Qi

Thanks for the report!

Right, currently the core NilAway does not reason too much about struct fields, especially struct initializations (e.g., creating empty structs or creating empty structs and then use a helper function to properly populate the struct fields).

However, we have an extension within NilAway that reason struct fields up to depth one (developed by @shubhamugare), making NilAway capable of handling code examples in this issue.

We have not enabled it since it increases build-time overhead to be slightly >5% (under our internal testing, but YMMV), meanwhile this capability brings more false positives to handle. We are actively working on applying more optimizations and false positive reduction to enable and integrate, but we are not there yet.

With all that said, if you would like to try it out, you can add a <nilaway struct enable> in the file's docstring to enable this extension for this file in particular. I have tried your example and it's able to show errors on the dereferences there. We'll add another configuration flag to make it easier for people to try it out :)

yuxincs avatar Aug 04 '23 16:08 yuxincs

Is this the same class of issue?

package main

import (
        "fmt"
)

type State struct {
        ok bool
}

func f(s *State) {
        s.ok = true
}

func main() {
        var s State
        f(&s)
        fmt.Println(s)
}

This generates an error:

/home/jrockway/test/nilness/main.go:12:6: error: Annotation on Param 0: 's' of Function f overconstrained:
        Must be NILABLE because it describes the value passed as arg `s` to func `f` at nilness/main.go:18:4, and that value is read from a variable that was never assigned to at nilness/main.go:17:6, where it is NILABLE
                AND
        Must be NONNIL because it describes the value read from the function parameter `s` of function `f` at nilness/main.go:12:8, and that value is passed to a field access at nilness/main.go:13:2, where it must be NONNIL

It's fixed by changing var s State to s := State{}, which seems like the same thing to me. The <nilaway struct enable> also fixes things, which leads me to believe that the struct field checking is the missing feature here.

jrockway avatar Sep 02 '23 19:09 jrockway