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(userspace/engine): fix memory leak #2877

Merged
merged 1 commit into from Oct 17, 2023

Conversation

therealbobo
Copy link
Contributor

@therealbobo therealbobo commented Oct 17, 2023

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

This fixes a memory leak: compiler.compile() allocates a sinp_filter but never destroys it.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(userspace/engine): fix memory leak

Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
@github-actions
Copy link

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

@therealbobo
Copy link
Contributor Author

@incertum maybe this is related to the memory leak that you found some time ago 🤔

Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

I'll gladly test deploy soon and let's see. Evidence that it also happened when using Falco only for the k8s plugins suggests this is in the right area of the code base.

At the same time tons of servers do not show any memory leaks ... This is the puzzling part. It's nothing consistent or easy to find so to say.

@poiana
Copy link

poiana commented Oct 17, 2023

LGTM label has been added.

Git tree hash: 83d6e339143405cec91af34ac27736cec7039c6c

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link

poiana commented Oct 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, therealbobo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@FedeDP
Copy link
Contributor

FedeDP commented Oct 17, 2023

/milestone 0.37.0

@poiana poiana added this to the 0.37.0 milestone Oct 17, 2023
@FedeDP
Copy link
Contributor

FedeDP commented Oct 17, 2023

/unhold

@poiana poiana merged commit b7cef5b into falcosecurity:master Oct 17, 2023
19 checks passed
@FedeDP
Copy link
Contributor

FedeDP commented Oct 24, 2023

/milestone 0.36.2

@poiana poiana modified the milestones: 0.37.0, 0.36.2 Oct 24, 2023
@FedeDP
Copy link
Contributor

FedeDP commented Oct 24, 2023

/milestone 0.37.0
There are conflicts; and i don't think this is that important to need custom code in the release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants