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: safeRequest memory leak #443

Merged
merged 4 commits into from
Nov 11, 2024
Merged

fix: safeRequest memory leak #443

merged 4 commits into from
Nov 11, 2024

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Nov 7, 2024

Issue

chalk running in as daemon with heartbeats had ever-increasing memory footprint

Description

Each heartbeat report, it would send HTTP request to chalk api. That was done via safeRequest from nimutils which would initialize SslContext. That context included all root CA certs/etc and would use ~1Mb on average. At the end of the request, it wasnt correctly discarded as nim would GC its reference however it would call into openssl functions to clear all the inner data-structures. Now we explicitly destroy the context after each request to ensure its memory is correctly cleared.

for more:

crashappsec/nimutils#81

Tests

Run long running process with heartbeats enabled and check its memory utilization.

Base automatically changed from make to main November 7, 2024 18:47
@miki725 miki725 force-pushed the memoryleak branch 2 times, most recently from 9b5a974 to df11dc8 Compare November 7, 2024 20:20
@miki725
Copy link
Contributor Author

miki725 commented Nov 8, 2024

con4m populates external commands via its callback however this means
that chalk indefinitely records external commands, even when reporting
state is otherwise flushed (e.g. for each heartbeat report)

as this can use unneeded memory, its better to clean it when wiping
reporting state
Some plugins maintain their own state which is currently persisted
even if its never needed anymore

For example lambda/ecs plugins cache AWS responses during exec report.
After exec report is sent, there is no more need to keep that state
as cloud metadata is not reported during heartbeats however its memory
is still used.

With the clear state callback each plugin can manage its own cache
as necessary not to waste memory.
safeRequest was creating new SSL context for each request however it was
not property getting cleaned up via destroyContext(). As such each
request was computing fresh SSL context which included all root certs
which would accumulate ~1Mb of memory
@miki725 miki725 changed the title wip: heartbeats memory leak fix: safeRequest memory leak Nov 8, 2024
@miki725 miki725 marked this pull request as ready for review November 8, 2024 18:28
@miki725 miki725 requested a review from viega as a code owner November 8, 2024 18:28
Copy link
Contributor

@viega viega left a comment

Choose a reason for hiding this comment

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

Excellent thanks, Miro.

@miki725 miki725 merged commit f916d85 into main Nov 11, 2024
4 checks passed
@miki725 miki725 deleted the memoryleak branch November 11, 2024 16:24
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.

2 participants