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 parsing invalid lines #579

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Conversation

matthiasr
Copy link
Contributor

The line

|h|#consumer:Kafka::SharedConfigurationConsumer,topic:shared_configuration_update,partition:1,consumer_group:tc_rc_us

caused a panic because the line parsing first splits by : and then failed to
find a | to split on.

Check that we get at least two "line parts" (i.e. splits around |) when we
expect them, and if not, gracefully reject the line instead of crashing.

Fixes #572.

Signed-off-by: Matthias Rampke [email protected]

@matthiasr
Copy link
Contributor Author

@GrgDev can you check my thinking here?

@m-barthelemy @twskipper if you have a chance, could you give this branch a spin to make sure it fixes the panic in your real world use cases, not just in unit tests?

pkg/line/line.go Outdated Show resolved Hide resolved
The line

```
|h|#consumer:Kafka::SharedConfigurationConsumer,topic:shared_configuration_update,partition:1,consumer_group:tc_rc_us
```

caused a panic because the line parsing _first_ splits by `:` and then failed to
find a `|` to split on.

Check that we get at least two "line parts" (i.e. splits around `|`) when we
expect them, and if not, gracefully reject the line instead of crashing.

Fixes #572.

Signed-off-by: Matthias Rampke <[email protected]>
@matthiasr matthiasr force-pushed the mr/issue-572/fix-panic branch from d6a7196 to 0c2c1d9 Compare September 15, 2024 11:47
Use go-error-style colons to add more details. Un-capitalize.

Signed-off-by: Matthias Rampke <[email protected]>
@m-barthelemy
Copy link

@GrgDev can you check my thinking here?

@m-barthelemy @twskipper if you have a chance, could you give this branch a spin to make sure it fixes the panic in your real world use cases, not just in unit tests?

I'd be happy to, but I don't know how to build an image from this project, had a quick look at the Dockerfile but it expects some already existing binary. Alternatively if there's an easy way for you to make a beta release image available on the Docker Hub, then it will be easy for me to test it on one of our non-production clusters.

Signed-off-by: Matthias Rampke <[email protected]>
@matthiasr
Copy link
Contributor Author

Here you go: prom/statsd-exporter:v0.27.2-pr579

@m-barthelemy
Copy link

Here you go: prom/statsd-exporter:v0.27.2-pr579

Thanks a ton!
Complete success so far:

  • no more crashes
  • can see ts=2024-09-17T03:04:11.508Z caller=line.go:234 level=debug msg="bad line: not enough '|'-delimited parts after first ':'" ... warnings being logged instead

@m-barthelemy
Copy link

Thanks again for the fix! Will there be a new release and new Docker images soon?

@matthiasr
Copy link
Contributor Author

Yes! I was on vacation, sorry :)

@matthiasr matthiasr merged commit a20db6e into master Oct 7, 2024
6 checks passed
@matthiasr matthiasr deleted the mr/issue-572/fix-panic branch October 7, 2024 13:26
@matthiasr
Copy link
Contributor Author

v0.27.2 is out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic in dogstatsd extended aggregation parsing
3 participants