Skip to content

Fix memory leak #3470

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.

Sign up for GitHub

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

Open
FireBurn wants to merge 2 commits into
base: v3/master
Choose a base branch
from
+18 −6
Open

Fix memory leak #3470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d50ded2
Fix memory leak in variable translation proxies
FireBurn Nov 21, 2025
26422d7
Fix memory leak in transaction rule removal lists
FireBurn Nov 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions headers/modsecurity/transaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,12 @@ class TransactionAnchoredVariables {
m_variableFilesTmpNames(t, "FILES_TMPNAMES"),
m_variableMultipartPartHeaders(t, "MULTIPART_PART_HEADERS"),
m_variableOffset(0),
m_variableArgsNames("ARGS_NAMES", &m_variableArgs),
m_variableArgsGetNames("ARGS_GET_NAMES", &m_variableArgsGet),
m_variableArgsPostNames("ARGS_POST_NAMES", &m_variableArgsPost)
m_pVariableArgsNames(std::make_unique<AnchoredSetVariableTranslationProxy>("ARGS_NAMES", &m_variableArgs)),
m_variableArgsNames(*m_pVariableArgsNames),
m_pVariableArgsGetNames(std::make_unique<AnchoredSetVariableTranslationProxy>("ARGS_GET_NAMES", &m_variableArgsGet)),
m_variableArgsGetNames(*m_pVariableArgsGetNames),
m_pVariableArgsPostNames(std::make_unique<AnchoredSetVariableTranslationProxy>("ARGS_POST_NAMES", &m_variableArgsPost)),
Comment on lines +208 to +212
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

PR description states m_variableArgs*Names were previously references bound to heap allocations (so destructors couldn’t free them). In this header, they were value members before this change, and this diff actually introduces reference members + heap allocation. Can you clarify the leak reproduction and why switching to heap allocation is necessary here? As-is, this change doesn’t match the described root cause.

Copilot uses AI. Check for mistakes.
m_variableArgsPostNames(*m_pVariableArgsPostNames)
Comment on lines +208 to +213
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

The new m_pVariable* + &m_variable* pattern adds three heap allocations per Transaction and is a fairly unusual ownership model (a unique_ptr plus a public reference alias). Previously these proxies were stored by value and would have been destroyed automatically with TransactionAnchoredVariables, so this change is unlikely to affect object lifetime; it mainly adds indirection/allocations. Consider keeping these as value members, or (if indirection is truly needed) store only std::unique_ptr members and update call sites to use .get()/* rather than introducing reference data members.

Copilot uses AI. Check for mistakes.
{ }

AnchoredSetVariable m_variableRequestHeadersNames;
Expand Down Expand Up @@ -291,9 +294,12 @@ class TransactionAnchoredVariables {

int m_variableOffset;

AnchoredSetVariableTranslationProxy m_variableArgsNames;
AnchoredSetVariableTranslationProxy m_variableArgsGetNames;
AnchoredSetVariableTranslationProxy m_variableArgsPostNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsNames;
AnchoredSetVariableTranslationProxy &m_variableArgsNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsGetNames;
AnchoredSetVariableTranslationProxy &m_variableArgsGetNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsPostNames;
Comment on lines +297 to +301
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

These new m_pVariableArgs* members are public and become part of the TransactionAnchoredVariables/Transaction API surface and object layout. If the intent is just to manage lifetime internally, consider keeping ownership private (e.g., make the pointer members private and expose only the existing m_variableArgs*Names fields or accessors), or avoid introducing the extra pointer members altogether by storing the proxies by value.

Suggested change
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsNames;
AnchoredSetVariableTranslationProxy &m_variableArgsNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsGetNames;
AnchoredSetVariableTranslationProxy &m_variableArgsGetNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsPostNames;
private:
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsGetNames;
std::unique_ptr<AnchoredSetVariableTranslationProxy> m_pVariableArgsPostNames;
public:
AnchoredSetVariableTranslationProxy &m_variableArgsNames;
AnchoredSetVariableTranslationProxy &m_variableArgsGetNames;

Copilot uses AI. Check for mistakes.
AnchoredSetVariableTranslationProxy &m_variableArgsPostNames;
};

class TransactionSecMarkerManagement {
Expand Down
6 changes: 6 additions & 0 deletions src/transaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ Transaction::~Transaction() {

m_rulesMessages.clear();

m_ruleRemoveById.clear();
m_ruleRemoveByIdRange.clear();
m_ruleRemoveByTag.clear();
m_ruleRemoveTargetById.clear();
m_ruleRemoveTargetByTag.clear();

intervention::free(&m_it);
intervention::clean(&m_it);

Expand Down