Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
hoptical opened this issue Dec 23, 2023 · 3 comments
Open
Labels
question Further information is requested

Comments

@hoptical
Copy link

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
Copy link
Contributor

@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?

@sonalmahajan15 sonalmahajan15 added the question Further information is requested label Dec 28, 2023
@hoptical
Copy link
Author

hoptical commented Jan 2, 2024

@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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants