Fix memory leaks in k8s audit and json events #1041
Merged
Conversation
This file contains 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
… default Signed-off-by: Lorenzo Fontana <lo@linux.com>
Signed-off-by: Lorenzo Fontana <lo@linux.com>
|
Here is the massif file of falco before the fix And this is after: |
|
Artifacts to test this can be found here: https://circleci.com/gh/falcosecurity/falco/341#artifacts/containers/0 |
|
/cc @leodido |
|
LGTM label has been added. Git tree hash: af23c3997319bfdcd33f6abdba6917bde3ea5bc9
|
leodido
approved these changes
Feb 6, 2020
mstemm
approved these changes
Feb 6, 2020
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kris-nova, leodido, mstemm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
great findings ! Yes we should be releasing falco now. @fntlnz |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes the memory leak bug reported by many users on the Falco slack.
WRT I started doing some memory analysis on my dev environment and noticed a pattern I didn’t like:
All the snapshots I was able to take were showing a linear increase in memory usage
Here’s an evidence of the leak, in how audit events are processed.
The problem I observed is that whenever the add_extracted_value function (https://github.com/falcosecurity/falco/blob/master/userspace/engine/json_evt.cpp#L629-L639) is called it adds an element to a list of strings. After that, old elements are never flushed. That leads to the memory leak we can see in the massif output above.
After some debugging, I noticed that add_extracted_value was only called when json_event_filter_check has m_uses_paths set to true. However, that value is never explicitly set to true in that codebase.
Then I noticed that whenever a new filtercheck is created (code : https://github.com/falcosecurity/falco/blob/master/userspace/engine/json_evt.cpp#L1379) the function allocate_new is called.
However, the created filter check does not contain false as a value for m_uses_paths but a numerical value. The reason for this is that while doing type conversion.
And since in C++ only zero is false and everything else is true that’s why we call add_extracted and have the memory leak.
This is the situation after my fix, as you can see the memory is pretty much always allocated and immediately released after processing.
Which issue(s) this PR fixes:
Fixes #1040
Special notes for your reviewer:
Does this PR introduce a user-facing change?: