Fix memory leak

[FireBurn]

This is a potential fix to #2848

It was AI generated by Google AI Studio

Here are it's workings:

Based on the Valgrind output and the provided code, the memory leak is located in the TransactionAnchoredVariables class (which is part of the Transaction object).

The stack trace indicates that memory allocated during the construction of AnchoredSetVariableTranslationProxy (specifically inside a std::function initialization) is never freed.

==27426== 1 bytes in 1 blocks are possibly lost ...
==27426== at 0x482BBB5: operator new(unsigned int) ...
==27426== by 0x94EB43A: ... function<...>::operator= ...
==27426== by 0x94EB43A: AnchoredSetVariableTranslationProxy ...
==27426== by 0x94EB43A: modsecurity::TransactionAnchoredVariables::TransactionAnchoredVariables(modsecurity::Transaction*) ...

In src/transaction.cc, the Transaction constructor initializes TransactionAnchoredVariables:

Transaction::Transaction(ModSecurity *ms, RulesSet *rules, const char *id,
    void *logCbData, const time_t timestamp)
    : ...
    TransactionAnchoredVariables(this) { // <--- Initialization here
    ...

And in src/variables/variable.h, the code accesses m_variableArgsNames as if it were an object (using &), not a pointer:

anchoredSetVariableTranslationProxy = &t->m_variableArgsNames;

This usage pattern combined with the leak indicates that TransactionAnchoredVariables declares its members (like m_variableArgsNames) as references (&) but initializes them using new in its constructor.

Because they are references, the TransactionAnchoredVariables destructor does not (and cannot) delete the objects they refer to. Consequently, the AnchoredSetVariableTranslationProxy objects (and their internal std::function members) are leaked when the Transaction is destroyed.

To fix this:
The members in TransactionAnchoredVariables should be changed from references (e.g., AnchoredSetVariableTranslationProxy &) to pointers (e.g., std::unique_ptr<AnchoredSetVariableTranslationProxy>), or they should be deleted manually in the TransactionAnchoredVariables destructor if raw pointers are used. Given the &t->member usage, they were likely intended to be value members or references binding to dynamic memory (which is the bug).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

This PR aims to address a reported memory leak during nginx reloads (ModSecurity issue #2848) by adjusting Transaction cleanup and changing how TransactionAnchoredVariables stores AnchoredSetVariableTranslationProxy instances.

Changes:

• Explicitly clears several rule-removal tracking lists in Transaction::~Transaction().
• Refactors TransactionAnchoredVariables to allocate AnchoredSetVariableTranslationProxy objects on the heap via std::unique_ptr, with public reference aliases preserving existing access patterns.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/transaction.cc	Adds explicit clear() calls for rule-removal lists during Transaction destruction.
headers/modsecurity/transaction.h	Replaces by-value translation proxy members with unique_ptr ownership plus reference aliases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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.

[airween]

@FireBurn,

first of all, thank you for this PR.

It was a bit suspicious, for eg. I don't understand the idea behind the new constructor's variable creating (you pass a non-existent reference to the new pointer type variable), but before I continue the understanding of the code, please could you show me a rule (a SecRule) or a configuration snippet and a request where I can check this memleak (with Valgrind).

Thank you!