fix memory patch, should close #2710

[liudongmiao]

No description provided.

[liudongmiao]

should same as #2580

[liudongmiao]

#2710

[mmckenna-yottaa]

If I may--It would seem that we should also restore the virtual destructor that the Rule class had in Modsec 3.0.4.

Since Modsec 3.0.5, the Rule class has not had an explicit destructor; and the compiler has therefore been defining an implicit nonvirtual destructor for the Rule class. The problem with the nonvirtual Rule destructor is that a call to that destructor will not exercise the subclass destructors. Restoring the virtual destructor for the Rule class will ensure that the subclass destructors are also called.

The problem of the nonvirtual Rule destructor will certainly manifest when the Rules container will destroy its internal member named "m_rules". In the Rules class, the m_rules destructor will call the Rule nonvirtual destructor, which will not exercise the Rule objects' subclass destructors. To ensure that the m_rules destructor exercises each Rule object's subclass destructors, we should restore the virtual destructor that the Rule class had in 3.0.4. This one-liner in "rule.h" should suffice:

virtual ~Rule() {}

In the above one-liner, the destructor requires no explicit logic because the Rule's virtual destructor will implicitly call the subclass destructors; and in the Rule class itself the internal members will do the right thing when they are destroyed. (As of 3.0.7, the Rule class's internal members are a shared_ptr and two integers.)

[mmckenna-yottaa]

The pull request at #2580 also suggested the one-liner:

virtual ~Rule() {}

[liudongmiao]

@mmckenna-yottaa From my test, either virtual would fix the issue, so I make a small change.

And I cannot understand, from shared_ptr's document, it's unnecessary to use virtual. However, in this case, it must use virtual, I still don't know why.

[mmckenna-yottaa]

The problem lies in the distinction between virtual and nonvirtual destructors.

In Modsec 3.0.4, the Rule class had a virtual destructor. In Modsec 3.0.5, the Rule destructor was removed from the source code. If the source code does not define a destructor for the Rule class (and if no parent class defines a virtual destructor), then the C++ compiler creates an implied nonvirtual destructor for the Rule class. The nonvirtual destructor can cause memory leaks because the nonvirtual destructor will not invoke the Rule object's subclass destructors.

The memory leak is illustrated in the m_rules member of the Rules class (in rules.h). When the m_rules vector is destroyed, the destructor for each shared_ptr object invokes exactly the destructor of the Rule class. Some of the Rule objects in the vector can be from subclasses of Rule. Unfortunately, the nonvirtual destructor for the Rule class will not invoke the destructors for the subclasses of the Rule object, thereby causing memory leaks.

To ensure that the Rule object's subclass destructors are invoked, we should explicitly declare the Rule destructor and make sure to include the "virtual" attribute. In other words, we should restore the virtual Rule destructor that was previously defined in Modsec 3.0.4. For the Rule class in Modsec 3.0.7, this one-line definition of the destructor will suffice in rule.h:

virtual ~Rule() {}

In Modsec 3.0.7, the destructor method for the Rule class does not require logic, because the Rule class's internal members (a shared_ptr and two ints) do the right thing when they are destroyed. We should nonetheless define the Rule destructor to declare its virtual nature.

[liudongmiao]

It's a bug introduced in commit 7a48245, actually this line: 7a48245#diff-a05323d17ad48a02cb12643dc6c352302d91fbfc7a4e46cd7e725e56f63b4589L97

When a RuleWithOperator is created, then it's stored into std::shared_ptr<RuleWithActions>, this std::shared_ptr know nothing about it's actual type RuleWithOperator, but RuleWithActions only. There are two options to fix it:

• use std::shared_ptr<RuleWithOperator>
• make destructor inRuleWithActions or it's superclass virtual.

@zimmerle or @martinhsv Could you verify this?

So, there are three solutions:

• add a virtual ~Rule (superclass of RuleWithActions)
• make ~RuleWithActions virtual, this commit
• use std::shared_ptr<RuleWithOperator> rule instead of std::shared_ptr<RuleWithActions> rule

@mmckenna-yottaa 's explanation is not true:

When the m_rules vector is destroyed, the destructor for each shared_ptr object invokes exactly the destructor of the Rule class.

Actually, the shared_ptr object is std::shared_ptr<RuleWithActions>, it will invoke RuleWithActions's destructor.

[liudongmiao]

@zimmerle @martinhsv @mmckenna-yottaa I just write a simple POC for solutions.

Name it as modsec-2728.cpp

#include <iostream>
#include <memory>
#include <vector>

#ifndef RULE_SHARED_PTR_CLASS
#define RULE_SHARED_PTR_CLASS RuleWithActions
#endif

class Rule {
public:
#ifdef RULE_VIRTUAL
    virtual ~Rule() {}
#else
    ~Rule() {}
#endif

    int getPhase() {
        return 0;
    }
};

class RuleWithActions : public Rule {
public:
#ifdef RULE_WITH_ACTIONS_VIRTUAL
    virtual ~RuleWithActions() {}
#else
    ~RuleWithActions() {}
#endif
};

class RuleWithOperator : public RuleWithActions {
public:
    RuleWithOperator() {
        std::cout << "Constructing RuleWithOperator" << std::endl;
    }

    ~RuleWithOperator() {
        std::cout << "Destructing RuleWithOperator" << std::endl;
    }
};

class Rules {
public:
    bool insert(const std::shared_ptr<Rule> &rule) {
        m_rules.push_back(rule);
        return true;
    }

    std::vector<std::shared_ptr<Rule> > m_rules;
};

class RulesSetPhases {
public:
    bool insert(std::shared_ptr<Rule> rule) {
        return m_rulesAtPhase[rule->getPhase()].insert(rule);
    }

private:
    Rules m_rulesAtPhase[1];
};

class Driver {
public:
    RulesSetPhases m_rulesSetPhases;

    int addSecRule(std::unique_ptr<RULE_SHARED_PTR_CLASS> r) {
        std::shared_ptr<RULE_SHARED_PTR_CLASS> rule(std::move(r));
        m_rulesSetPhases.insert(std::move(rule));
        return 0;
    }
};

int main() {
    Driver driver;
    std::unique_ptr<RuleWithOperator> rule(new RuleWithOperator());
    driver.addSecRule(std::move(rule));
}

And a simple Makefile:

source = modsec-2728.cpp
CXX = g++
CXXFLAGS = -std=c++11 -Wall -Wextra

default: modsec-2728 modsec-2728-virtual-rule modsec-2728-virtual-rule-with-actions modsec-2728-shared-ptr-rule-with-operator

modsec-2728: $(source)
	$(CXX) $(CXXFLAGS) $< -o $@

modsec-2728-virtual-rule: $(source)
	$(CXX) $(CXXFLAGS) $< -o $@ -DRULE_VIRTUAL

modsec-2728-virtual-rule-with-actions: $(source)
	$(CXX) $(CXXFLAGS) $< -o $@ -DRULE_WITH_ACTIONS_VIRTUAL

modsec-2728-shared-ptr-rule-with-operator: $(source)
	$(CXX) $(CXXFLAGS) $< -o $@ -DRULE_SHARED_PTR_CLASS=RuleWithOperator

clean:
	rm -rf modsec-2728 modsec-2728-*

Then you can run:

for x in modsec-2728 modsec-2728-*; do echo $x; ./$x; done

It will show:

modsec-2728
Constructing RuleWithOperator
modsec-2728-shared-ptr-rule-with-operator
Constructing RuleWithOperator
Destructing RuleWithOperator
modsec-2728-virtual-rule
Constructing RuleWithOperator
Destructing RuleWithOperator
modsec-2728-virtual-rule-with-actions
Constructing RuleWithOperator
Destructing RuleWithOperator

[iammattmartin]

We've tested this from the reference before and this did not solve the memory issues for us.

[liudongmiao]

We've tested this from the reference before and this did not solve the memory issues for us.

@iammattmartin I have post poc codes in #2710 , and codes here for addressing.
You should test whether codes in 2710 works, and codes here works.

If codes in #2710 seems fixed, then you should check other patches.

And as addressing here, you can use old modsecurity to check, like before 3.0.4.

[mmckenna-yottaa]

liudongmiao is correct. Executing the code that he posted on Aug 29, 2022, 07:43 GMT, we see that the destructor for

 std::vector<std::shared_ptr<Rule> > m_rules

does invoke the destructor for subclass RuleWithOperator, even when the Rule destructor is not virtual.

liudongmiao's code settles the question. That said, please bear with me just a little more...

If we use a regular Rule pointer and the Rule destructor is not virtual, then the subclass destructor is not invoked:

 Rule * rulePtr = new RuleWithOperator();
 delete rulePtr;

but we see that the std::shared_ptr<Rule> does invoke the subclass destructor. What is going on? The implementation notes at https://en.cppreference.com/w/cpp/memory/shared_ptr might offer a clue. The notes refer to a deleter that might point to the object's destructor.

Let me then qualify my recommendation, for what it's worth:

If we do not wish to depend on the shared_ptr implementation in member Rules::m_rules, then we should define a virtual Rule destructor. ModSecurity 3.0.4 explicitly defined a virtual ~Rule destructor, but ModSecurity 3.0.5 made the Rule destructor implicit and therefore non-virtual.

[liudongmiao]

@mmckenna-yottaa

shared_ptr stores original delete Destructor.

Rule * rulePtr = new RuleWithOperator();
delete rulePtr; // cannot recognize the actual type of rulePtr is RuleWithOperator

Rule *rulePtr = new RuleWithOperator();
std::shared_ptr<Rule> rule(static_cast<RuleWithOperator *>(rulePtr));
// when rule is deconstructing, shared_ptr know it's RuleWithOperator (stores in shared_ptr)

// should be write like this
std::shared_ptr<Rule> rule(new RuleWithOperator());

There are several solution, I just pick the smallest change one.

[martinhsv]

The discussion around shared_ptr and it automatically calling destructors of derived classes even if the base class is non-virtual is quite interesting.

To buttress the findings, I'll note this explanation: https://stackoverflow.com/questions/3899790/shared-ptr-magic . Although the top-rated answer states "... the C++11 standard also requires this behaviour.", reading through the standard, the wording seems a bit obscure.

Nevertheless, I would agree with @mmckenna-yottaa 's recommendation that the Rule class ought to have a virtual destructor . Not only is the lack of a virtual destructor an issue in the case of new use of raw pointers (which the ModSecurity project should seek to avoid), but also because unique_ptr does not perform the same 'magic' (and we probably do want to make more use of unique_ptr in future).

This change to add a virtual destructor to the Rule class was implemented recently via #2801 .

[martinhsv]

This pull request sought to address the same three issues as #2580 :

1. the lack of a virtual destructor for the Rule class
2. delete of transformations vector
3. parser-filename issues

(1) and (2) have already been implemented via PR #2801, while the solution for (3) in this PR has the same problem as discussed here: #2580 (comment) ).

Given that, I'll go ahead and close this item.

@liudongmiao : I'm open to changing how the delete of the transformations vector occurs, but given where we are now, I think doing so via a fresh PR problem makes the most sense (for example, the reason to make the change now would be more like 'misc code improvement' rather resolving an actual memory leak on reload). Also, if we do proceed that way, making the comparable change to the actions vector should be actively considered simultaneously so that we don't have two different memory-management models active in the same section of code. Feel free to submit a fresh such PR, if you like (but, no obligation, of course).