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(inputs.conntrack): resolve segfault when setting collect field #12603

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

deric
Copy link
Contributor

@deric deric commented Feb 2, 2023

Enabling stats feature in conntrack input by setting collect field (affected versions v1.25.0, v1.25.1) causes segfault

minimal config:

[[inputs.conntrack]]
collect = ["all"]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x1387053]

goroutine 70 [running]:
github.com/influxdata/telegraf/plugins/inputs/conntrack.(*Conntrack).Gather(0xc00046a000, {0x6cb41a0, 0xc0000c33a0})
        /go/src/github.com/influxdata/telegraf/plugins/inputs/conntrack/conntrack.go:109 +0x133
github.com/influxdata/telegraf/agent.(*Agent).testRunInputs.func2(0xc0007c2730)
        /go/src/github.com/influxdata/telegraf/agent/agent.go:419 +0x2f8
created by github.com/influxdata/telegraf/agent.(*Agent).testRunInputs
        /go/src/github.com/influxdata/telegraf/agent/agent.go:388 +0xc

fixes: #8955

The c.ps variable was not properly initialized, however during tests we're passing a mock instance, so this error was not covered by automated tests.

  • Add lazy initialization when collect config is used
  • collect array is checked for supported values
  • Config initialization is moved out of Gather() method to Init()
  • Add test case to cover the system.PS initialization
  • Fix collecting both "all" and per-CPU metrics at the same time

@deric deric force-pushed the conn_segfault branch 3 times, most recently from da51c85 to b2e327d Compare February 2, 2023 17:01
@powersj powersj changed the title fix(inputs.conntrack) Segfault when collect is configured (resolves #8955) fix(inputs.conntrack): resolve segfault when setting collect field Feb 2, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to put this up with a fix. Can you take a look at my two comments?

plugins/inputs/conntrack/conntrack.go Outdated Show resolved Hide resolved
plugins/inputs/conntrack/conntrack.go Outdated Show resolved Hide resolved
@deric deric force-pushed the conn_segfault branch 2 times, most recently from 872f430 to d06b036 Compare February 3, 2023 02:50
@deric
Copy link
Contributor Author

deric commented Feb 3, 2023

@powersj Thanks for looking into this. I'm sorry, the first PR was simply bad.

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Very nice PR @deric! Just one small comment for the Init() code...

plugins/inputs/conntrack/conntrack.go Outdated Show resolved Hide resolved
@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 6, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the quick update @deric!

@srebhan srebhan merged commit 0ea50fa into influxdata:master Feb 7, 2023
powersj pushed a commit that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support collecting conntrack stats
3 participants