flightplan: fix various potential nil panics #119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When testing scenario filters with negations for variants that didn't exist for some scenarios, I ran into a panic in the Matrix implementation due to not checking for nil and handling it appropriately in our exclusions. After fixing it I decided to use the `nilaway`[0] static analysis tool to verify the entire `flightplan` package. The tool itself definitely has a few rough edges, and there were times when it was confused and I had to do things more than once in a function to please it, but overall it found several useful improvements. It was fairly time consuming so I scoped this to just the `flightplan` package for now. Eventually it might be nice to enable for the entire program but we'd either need to architect away our use of global variables for the UI, server client, and server, or we'd need them to reintroduce per line `//nolint` directives[1]. It also has a few show stopper bugs like not being able to handle listening on a nil channel in a select statement.[2] A tool with a promising and useful future but needs more time for us to enforce. [0] https://github.com/uber-go/nilaway [1] uber-go/nilaway#126 [2] uber-go/nilaway#143 Signed-off-by: Ryan Cragun <me@ryan.ec>
ryancragun
added
the
changelog/bug
ryancragun
commented
Nov 30, 2023
| @@ -115,7 +115,7 @@ func (e Element) Equal(other Element) bool { | |||
|
|
|||
| // Match determines if Exclude directive matches the given Vector. | |||
| func (ex *Exclude) Match(vec *Vector) bool { | |||
| if vec == nil { | |||
| if ex == nil || vec == nil { | |||
There was a problem hiding this comment.
This was the bug that led to the entire PR
rebwill
approved these changes
Dec 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing scenario filters with negations for variants that didn't exist for some scenarios, I ran into a panic in the Matrix implementation due to not checking for nil and handling it appropriately in our exclusions. After fixing it I decided to use the
nilaway[0] static analysis tool to verify the entireflightplanpackage.The tool itself definitely has a few rough edges, and there were times when it was confused and I had to do things more than once in a function to please it, but overall it found several useful improvements.
It was fairly time consuming so I scoped this to just the
flightplanpackage for now. Eventually it might be nice to enable for the entire program but we'd either need to architect away our use of global variables for the UI, server client, and server, or we'd need them to reintroduce per line//nolintdirectives[1].It also has a few show stopper bugs like not being able to handle listening on a nil channel in a select statement.[2] A tool with a promising and useful future but needs more time for us to enforce.
[0] https://github.com/uber-go/nilaway
[1] uber-go/nilaway#126
[2] uber-go/nilaway#143