Don't leak or race in FreeBSD devstat collector#396
Merged
discordianfish merged 5 commits intoprometheus:masterfrom Jan 5, 2017
dominikh:bsd-memleak
Merged
Don't leak or race in FreeBSD devstat collector#396discordianfish merged 5 commits intoprometheus:masterfrom dominikh:bsd-memleak
discordianfish merged 5 commits intoprometheus:masterfrom
dominikh:bsd-memleak
Conversation
Member
|
Oh great, thanks! Unfortunately I've just merged a PR to change the metrics to const metrics so some conflicts need to be resolved. Can you rebase this? |
Contributor
Author
|
Sure, will do tomorrow. |
The memory allocated by calloc was never freed. Since the devinfo struct never leaves the function, anyway, we might as well just allocate it on the stack.
Embedding 100 lines of code in a comment doesn't make for good reading, editing or code quality.
Querying the number of devices separately from the device list itself is racy. Devices may be added or removed between the two calls; and removed devices would lead to a segfault.
The devstat API expects us to reuse one devinfo for many invocations of devstat_getstats. In particular, it allocates and resizes memory referenced by devinfo.
Contributor
Author
|
@discordianfish PTAL |
grobie
reviewed
Jan 5, 2017
| @@ -0,0 +1,66 @@ | |||
| // +build !nodevstat | |||
Member
There was a problem hiding this comment.
Please include our license header at the top.
grobie
approved these changes
Jan 5, 2017
Member
grobie
left a comment
grobie
left a comment
There was a problem hiding this comment.
Haven't looked at the C code, but I trust you tested it.
Member
Contributor
Author
|
@discordianfish Already done in 9847257 |
Member
|
Gna.. github.. Thanks! |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The FreeBSD devstat collector was leaking memory (calloc with no matching free as well as memory allocated by devstat_getdevs). It was also racing, by querying the number of devices and the actual list of devices separately.
The same flaws exist in the DragonflyBSD collector. Someone with access to a machine for testing might want to port the fixes over.