search

uber-go / nilaway

Static analysis tool to detect potential nil panics in Go code
Apache License 2.0
3.04k stars 59 forks source link

Nilaway doesn't recognize a Nil case caused by subreconciler package #159

Open hoptical opened 8 months ago

hoptical commented 8 months ago

We had faced a nil pointer error in this line of the project in the runtime. I checked it out with Nilaway with this command since I knew it was caused by the subreconciler package:

nilaway -include-pkgs github.com/snapp-incubator/s3-operator,github.com/opdev/subreconciler ./...

However, Nilaway didn't recognize this part as a nil pointer and dangerous part of the code. I just wanted to mention it; you might find it helpful for further contributions to the package.

Thanks in advance.

sonalmahajan15 commented 8 months ago

@hoptical, thank you for bringing this to our attention. We've implemented error handling logic in NilAway. In this setup, if the error return of a function is nil, the function is considered successful, and its non-error return values are tracked for nilability. Conversely, if the error return is non-nil, the function is deemed unsuccessful, and its non-error return values are not tracked. Also, it's crucial to appropriately guard the dereferencing of return values of such functions. NilAway will raise an error if any violation is detected in this flow.

The line in question indicates a nil source for a non-error return in the presence of a non-nil error return. This situation is acceptable if, at the call site, the returns are correctly guarded. Could you point us to the code snippet calling ensureSecret() where the dereferencing of the return ctrl.Result led to the nil panic?

hoptical commented 8 months ago

@sonalmahajan15 Thanks for your explanation. The nil panic was happening in this line caused by ensureAdminSecret subreconciled in here. The ensureAdminSecret is calling the ensureSecret function.

sonalmahajan15 commented 8 months ago

Thanks for the context, @hoptical. It appears we're dealing with a challenging error return passing scenario that NilAway currently doesn't support (as documented in issue #101).

To further investigate, could you please share the stack trace here? While the result of ensureAdminSecret doesn't seem to be dereferenced at the line you highlighted, it is dereferenced via nested calls in the body of the function. Having the stack trace would facilitate a clearer understanding of the nil flow. Thanks!