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

Fix panic when Hearbeat monitor initialization fails twice #25073

Conversation

jsoriano
Copy link
Member

What does this PR do?

Prevents a panic happening when ICMP initialization fails twice.

Initialization was only tried once. Second time a nil object and a nil error are returned, producing nil pointer dereferences later.

With static configurations the first failure inmediatelly stops heartbeat, so this is not a problem, but with autodiscover multiple initialization attempts may happen.

Why is it important?

Prevent unexpected panics when introducing ICMP configurations through autodiscover.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

  • Configure two ICMP monitors using autodiscover without NET_RAW capabilities.
  • Failures should be printed, but Heartbeat should continue running.

Related issues

Logs

For the record, stack trace printed when this issue happens:

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

goroutine 74 [running]:
github.com/elastic/beats/v7/heartbeat/monitors/active/icmp.(*stdICMPLoop).checkNetworkMode(0x0, 0x29c9292, 0x2, 0x0, 0xc000100400)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/active/icmp/stdloop.go:170 +0x5d
github.com/elastic/beats/v7/heartbeat/monitors/active/icmp.(*jobFactory).makePlugin(0xc000954a20, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2d0eda0, 0x3e69478)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/active/icmp/icmp.go:94 +0x7a
github.com/elastic/beats/v7/heartbeat/monitors/active/icmp.create(0x29ca5fc, 0x4, 0xc0008151a0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/active/icmp/icmp.go:62 +0x2b5
github.com/elastic/beats/v7/heartbeat/monitors/plugin.(*PluginFactory).Create(...)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/plugin/plugin.go:164
github.com/elastic/beats/v7/heartbeat/monitors.newMonitorUnsafe(0xc0008151a0, 0xc000010020, 0x0, 0x0, 0x0, 0x0, 0xab7702fa0e1a4b9e, 0x0, 0x0)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/monitor.go:160 +0x3af
github.com/elastic/beats/v7/heartbeat/monitors.newMonitor(0xc0008151a0, 0xc000010020, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc00061dc48, 0x14c2490)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/monitor.go:105 +0x6c
github.com/elastic/beats/v7/heartbeat/monitors.checkMonitorConfig(0xc0008151a0, 0xc000010020, 0x0, 0x0, 0xc000738140)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/monitor.go:76 +0x53
github.com/elastic/beats/v7/heartbeat/monitors.(*RunnerFactory).CheckConfig(0xc00011c000, 0xc0008151a0, 0x0, 0x0)
	/home/jaime/gocode/src/github.com/elastic/beats/heartbeat/monitors/factory.go:85 +0x47
github.com/elastic/beats/v7/libbeat/autodiscover.(*Autodiscover).handleStart(0xc00022a750, 0xc0008153b0, 0x29cbcf7)
	/home/jaime/gocode/src/github.com/elastic/beats/libbeat/autodiscover/autodiscover.go:210 +0x484
github.com/elastic/beats/v7/libbeat/autodiscover.(*Autodiscover).worker(0xc00022a750)
	/home/jaime/gocode/src/github.com/elastic/beats/libbeat/autodiscover/autodiscover.go:140 +0x4ea
created by github.com/elastic/beats/v7/libbeat/autodiscover.(*Autodiscover).Start
	/home/jaime/gocode/src/github.com/elastic/beats/libbeat/autodiscover/autodiscover.go:121 +0x111
jaime@voyager:~/gocode/src/github.com/elastic/beats/heartbeat$ 

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.
@jsoriano jsoriano added review Team:Integrations Label for the Integrations team Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Apr 14, 2021
@jsoriano jsoriano requested a review from a team April 14, 2021 11:21
@jsoriano jsoriano self-assigned this Apr 14, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime (Team:Uptime)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 14, 2021
@jsoriano jsoriano added the needs_backport PR is waiting to be backported to other branches. label Apr 14, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 14, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #25073 updated

  • Start Time: 2021-04-14T11:24:11.586+0000

  • Duration: 36 min 27 sec

  • Commit: 3a20638

Test stats 🧪

Test Results
Failed 0
Passed 3337
Skipped 76
Total 3413

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 3337
Skipped 76
Total 3413

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Not super familiar with this specific part of the codebase but change itself looks good to me.

@jsoriano jsoriano merged commit 1d858bd into elastic:master Apr 14, 2021
@jsoriano jsoriano deleted the heartbeat-hints-autodiscover-panic-check-config branch April 14, 2021 15:44
jsoriano added a commit to jsoriano/beats that referenced this pull request Apr 14, 2021
…5073)

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.

(cherry picked from commit 1d858bd)
@jsoriano jsoriano added v7.13.0 and removed needs_backport PR is waiting to be backported to other branches. labels Apr 14, 2021
jsoriano added a commit to jsoriano/beats that referenced this pull request Apr 14, 2021
…5073)

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.

(cherry picked from commit 1d858bd)
jsoriano added a commit that referenced this pull request Apr 14, 2021
…25089)

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.

(cherry picked from commit 1d858bd)
jsoriano added a commit that referenced this pull request Apr 14, 2021
…25090)

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.

(cherry picked from commit 1d858bd)
v1v added a commit to v1v/beats that referenced this pull request Apr 15, 2021
* upstream/master:
  packer cache support for the 7.x and 7.latestMinor branches (elastic#25091)
  Remove EventFetcher and EventsFetcher interface (elastic#25093)
  Update go-structform to 0.0.8 (elastic#25051)
  Update copy_fields.asciidoc (elastic#25053)
  [elastic-agent] ensure container is backwards compatible (elastic#25092)
  Add --fleet-server-service-token. Rename --fleet-server to --fleet-server-es. (elastic#25083)
  Add cgroup.cpuacct percentages (elastic#25057)
  Add tests for truncated and symlinked files in filestream input (elastic#24425)
  Fix panic when Hearbeat monitor initialization fails twice (elastic#25073)
  [Filebeat][httpjson] Change append transform to initiate new fields as a slice (elastic#25074)
  Osquerybeat: Result values type translation (elastic#25012)
  Update Osquerybeat spec to get it downloading from the correct artifactory path (elastic#25076)
  Fix changelog (elastic#25079)
  Strip Azure EventHub connection string in debug logs (elastic#25066)
  Change googlecloud to gcp in field names (elastic#25038)
  Bump stack version to 7.12.0 for testing (elastic#24957)
  packer-cache: cache the existing docker images on ARM and some more (elastic#25068)
  Disable logstash TestFetch flaky test (elastic#25044)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…5073) (elastic#25090)

Initialization was only tried once. Second time a nil object and a nil
error are returned, producing nil pointer dereferences later.

(cherry picked from commit 841c82d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Integrations Label for the Integrations team Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.12.1 v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants