Skip to content

Conversation

@FireBurn
Copy link

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).

@FireBurn FireBurn force-pushed the issue-2848 branch 4 times, most recently from d9c7725 to 4b360b3 Compare November 22, 2025 14:47
The members m_variableArgsNames, m_variableArgsGetNames, and m_variableArgsPostNames were allocated using new but stored as references. This caused a memory leak because their destructors were never called.

This change wraps the allocations in std::unique_ptr to ensure proper cleanup while keeping the reference members to maintain compatibility with existing code syntax.
Valgrind reported memory leaks in m_ruleRemoveTargetByTag and related containers when ctl actions were used.

This change explicitly clears m_ruleRemoveById, m_ruleRemoveByIdRange, m_ruleRemoveByTag, m_ruleRemoveTargetByTag, and m_ruleRemoveTargetById in the Transaction destructor to ensure all allocated memory is freed.
@sonarqubecloud
Copy link

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.

1 participant