-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
filebeat/input/udp - Fix panic in linux input metrics #35068
filebeat/input/udp - Fix panic in linux input metrics #35068
Conversation
A panic occurred while parsing /proc/net/udp and reaching the final empty line. This occurred on Linux when the desired socket was not found in the list. Fixes elastic#35064
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link in the Changelog is the wrong issue, except that LGTM! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
just a suggestion on documenting why 12 fields and making the error message consistent with inputs.
@@ -268,7 +268,7 @@ func procNetUDP(addr []string) (rx, drops int64, err error) { | |||
} | |||
for _, l := range lines[1:] { | |||
f := bytes.Fields(l) | |||
if contains(f[1], addr) { | |||
if len(f) > 12 && contains(f[1], addr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12
is a little "magical", how about at least a link to kernel docs for why there should be this many fields?
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/net/ipv4/udp.c?h=v4.14.312#n2764
also fmt.Errorf above should probably use the path
variable instead of "/proc/net/udp" now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I checked for 12 is because it is highest field indexed (e.g. f[12]
) by this section of code.
fmt.Errorf above should probably use the path
Yeah, that thought crossed my mind. I didn't do it because it was parameterized only for testing purposes. But if you had the thought too then I will change it.
A panic occurred while parsing /proc/net/tcp and reaching the final empty line. This occurred on Linux when the desired socket was not found in the list. Fixes elastic#35064
A panic occurred while parsing /proc/net/udp and reaching the final empty line. This occurred on Linux when the desired socket was not found in the list. Fixes #35064
What does this PR do?
A panic occurred while parsing /proc/net/udp and reaching the final empty line. This occurred on Linux when the desired socket was not found in the list.
Fixes #35064
Why is it important?
The panic causes Filebeat to exit.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Related issues