Skip to content

fix: prevent nil pointer dereference in health checks#394

Merged
stefanprodan merged 1 commit intofluxcd:mainfrom
makkes:prevent-npes
Jul 27, 2021
Merged

fix: prevent nil pointer dereference in health checks#394
stefanprodan merged 1 commit intofluxcd:mainfrom
makkes:prevent-npes

Conversation

@makkes
Copy link
Copy Markdown
Member

@makkes makkes commented Jul 26, 2021

The lastStatus map now stores event.ResourceStatus values instead
of pointers to them, preventing nil pointers being dereferenced by
accident.

refs #374

Signed-off-by: Max Jonas Werner mail@makk.es

@makkes
Copy link
Copy Markdown
Member Author

makkes commented Jul 26, 2021

@stefanprodan as promised.

@hiddeco
Copy link
Copy Markdown
Member

hiddeco commented Jul 26, 2021

Wouldn't this result in L92-100 tripping because it now deals with an initialized but empty object, and wouldn't it thus be better to keep the pointer list, but deal with nil values there?

Only non-nil values are added to the list, so this will not happen, but I still think the dereference makes little sense.

(In general I think, pointer object lists often use pointers because nil has a special meaning, and you want to maintain this information for as long as you may need it).

Copy link
Copy Markdown
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses all my (personal) previous concerns, thank you @makkes 💯

@hiddeco
Copy link
Copy Markdown
Member

hiddeco commented Jul 26, 2021

DCO signoff (and while you are at it, a smash of) commits is still required.

When checking the health status of each declared resource, kstatus
might return nil for certain resources (for whatever reason). In that
case, this information is now conveyed in the health status event.

fluxcd#374

Signed-off-by: Max Jonas Werner <mail@makk.es>
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @makkes

@stefanprodan stefanprodan added area/kstatus Health checking related issues and pull requests bug Something isn't working labels Jul 27, 2021
@stefanprodan stefanprodan merged commit 9d66fc7 into fluxcd:main Jul 27, 2021
@makkes makkes deleted the prevent-npes branch July 27, 2021 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kstatus Health checking related issues and pull requests bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants