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.

Sign up for GitHub

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

Jump to bottom

[release-1.7] Fix panic: "nil pointer dereference" #2012

Merged
merged 1 commit into from
Jun 19, 2022
Merged

[release-1.7] Fix panic: "nil pointer dereference" #2012

merged 1 commit into from
Jun 19, 2022

Conversation

nunnatsa
Copy link
Collaborator

@nunnatsa nunnatsa commented Jun 19, 2022 •
edited

The software craches when updating the prometheusRule, because then we're
trying to send a nil HyperConverged pointer to a function as an interface.
The function does check for nil, but this is not working, because
interface(nil) != nil.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x146c420]

goroutine 1176 [running]:
github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1.(*HyperConverged).GetObjectKind(0x1867900?)
	<autogenerated>:1
k8s.io/client-go/tools/reference.GetReference(0x18cdf45?, {0x1b25fb8, 0x0})
	/remote-source/app/vendor/k8s.io/client-go/tools/reference/ref.go:59 +0x137
k8s.io/client-go/tools/record.(*recorderImpl).generateEvent(0xc000a5d8c0, {0x1b25fb8?, 0x0}, 0x1b464c8?, {0x18a076f, 0x7}, {0x18a460c, 0xb}, {0xc00333a7d0, 0x42})
	/remote-source/app/vendor/k8s.io/client-go/tools/record/event.go:330 +0x76
k8s.io/client-go/tools/record.(*recorderImpl).Event(0x73?, {0x1b25fb8?, 0x0?}, {0x18a076f?, 0x27?}, {0x18a460c?, 0x0?}, {0xc00333a7d0?, 0x1?})
	/remote-source/app/vendor/k8s.io/client-go/tools/record/event.go:355 +0x4f
sigs.k8s.io/controller-runtime/pkg/internal/recorder.(*lazyRecorder).Event(0xc000799300, {0x1b25fb8, 0x0}, {0x18a076f, 0x7}, {0x18a460c, 0xb}, {0xc00333a7d0, 0x42})
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder.go:155 +0xcb
github.com/kubevirt/hyperconverged-cluster-operator/pkg/util.eventEmitter.EmitEvent({{0x1b38740?, 0xc000799300?}, 0xc000144000?, 0xc000839680?}, {0x1b25fb8?, 0x0?}, {0x18a076f, 0x7}, {0x18a460c, 0xb}, ...)
	/remote-source/app/pkg/util/event_emmiter.go:41 +0x114
github.com/kubevirt/hyperconverged-cluster-operator/controllers/alerts.AlertRuleReconciler.updateAlert({{0x1b3e5d0, 0xc0001aee10}, {0xc000a083e0, 0xd}, 0xc0000be500, 0xc0030f7680, {0x1b260d0, 0x277d0c0}, 0xc0001403f0}, 0xc002def7a0, ...)
	/remote-source/app/controllers/alerts/alerts.go:165 +0x564
github.com/kubevirt/hyperconverged-cluster-operator/controllers/alerts.(*AlertRuleReconciler).Reconcile(0xc000116410, 0xc002def7a0)
	/remote-source/app/controllers/alerts/alerts.go:109 +0x1e5
github.com/kubevirt/hyperconverged-cluster-operator/controllers/hyperconverged.(*ReconcileHyperConverged).Reconcile(0xc0000b21c0, {0x1b393f0, 0xc003303d40}, {{{0xc000054013?, 0x17c4ee0?}, {0xc000afc240?, 0x30?}}})
	/remote-source/app/controllers/hyperconverged/hyperconverged_controller.go:277 +0x279
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0xc00023cf20, {0x1b393f0, 0xc003303c80}, {{{0xc000054013?, 0x17c4ee0?}, {0xc000afc240?, 0x409514?}}})
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:114 +0x27e
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc00023cf20, {0x1b39348, 0xc000996d00}, {0x16f41e0?, 0xc00193a520?})
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:311 +0x349
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc00023cf20, {0x1b39348, 0xc000996d00})
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:266 +0x1d9
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:227 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	/remote-source/app/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:223 +0x31c

This PR fixes the bug by not sending the nil interface but an actual
nil, and also changes the check to use reflection in order to make sure
that the function parameter is nil.

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

  • PR Message
  • Commit Messages
  • How to test
  • Unit Tests
  • Functional Tests
  • User Documentation
  • Developer Documentation
  • Upgrade Scenario
  • Uninstallation Scenario
  • Backward Compatibility
  • Troubleshooting Friendly

Release note:

Fix sw crash

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 19, 2022
@coveralls
Copy link
Collaborator

coveralls commented Jun 19, 2022 •
edited

Pull Request Test Coverage Report for Build 2524003115

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.888%
Totals Coverage Status
Change from base Build 2515718514: 0.0%
Covered Lines: 4285
Relevant Lines: 5108
💛 - Coveralls

@nunnatsa
[release-1.7] Fix "nil pointer exception"
5936d0c 
The software craches when updating a metric resource, because then we're
trying to send the HyperConverged pointer to a function as an interface.
The function does check for nil, but this is not working, because
interface(nil) != nil.

This PR fixes the bug by not sending the nil interface but an actual
nil, and also changes the check to use reflection in order to make sure
that the function parameter is nil.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
@nunnatsa nunnatsa force-pushed the r17-fix_nil_pointer_exception branch from ea6057d to 5936d0c Compare June 19, 2022 12:44
@sonarcloud
Copy link

sonarcloud bot commented Jun 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@nunnatsa nunnatsa changed the title [release-1.7] Fix "nil pointer exception" [release-1.7] Fix panic: "nil pointer dereference" Jun 19, 2022
@hco-bot
Copy link
Collaborator

hco-bot commented Jun 19, 2022

hco-e2e-upgrade-prev-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-azure
hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-sno-azure, ci/prow/hco-e2e-upgrade-prev-index-azure

In response to this:

hco-e2e-upgrade-prev-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-azure
hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@machadovilaca
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2022
orenc1
orenc1 approved these changes Jun 19, 2022
View reviewed changes
Copy link
Collaborator

@orenc1 orenc1 left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: orenc1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2022
orenc1
orenc1 reviewed Jun 19, 2022
View reviewed changes
pkg/util/event_emmiter.go
if object != nil {
// checking object != nil does not work because object is an interface, and nil interface instance is not nil.
// We need to check that it's actually nil, using reflection.
if t := reflect.ValueOf(object); t.Kind() != reflect.Ptr || !t.IsNil() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not that it make a difference, but maybe use the exact same code as in main?
https://github.com/kubevirt/hyperconverged-cluster-operator/pull/2011/files#diff-04c60da2651653a56d9a3e11edaa1694261a63200172901315131ea465758747R44
(i.e. Pointer vs. Ptr)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't do it before. Pointer added at go 1.18. This branch was compiled with 1.17. I just fixed it, but it wasn't ready on time.

@openshift-ci
Copy link

openshift-ci bot commented Jun 19, 2022 •
edited

@nunnatsa: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/hco-e2e-image-index-sno-azure 5936d0c link false /test hco-e2e-image-index-sno-azure
ci/prow/hco-e2e-upgrade-prev-index-azure 5936d0c link true /test hco-e2e-upgrade-prev-index-azure
ci/prow/hco-e2e-upgrade-prev-index-sno-aws 5936d0c link false /test hco-e2e-upgrade-prev-index-sno-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@hco-bot
Copy link
Collaborator

hco-bot commented Jun 19, 2022

hco-e2e-upgrade-prev-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-aws

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-index-sno-aws

In response to this:

hco-e2e-upgrade-prev-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubevirt-bot kubevirt-bot merged commit 277b57d into kubevirt:release-1.7 Jun 19, 2022
@nunnatsa nunnatsa deleted the r17-fix_nil_pointer_exception branch September 28, 2022 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orenc1 orenc1 orenc1 approved these changes

@tiraboschi tiraboschi Awaiting requested review from tiraboschi

Assignees

@machadovilaca machadovilaca

@orenc1 orenc1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nunnatsa @coveralls @hco-bot @kubevirt-bot @machadovilaca @orenc1