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 some memory leaks for parsing & cleaning up rules #2580

Closed
wants to merge 1 commit into from

Conversation

Abce
Copy link

@Abce Abce commented Jun 18, 2021

No description provided.

@kudrom kudrom mentioned this pull request Jul 7, 2021
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Jul 9, 2021
@zimmerle zimmerle self-assigned this Jul 9, 2021
@zimmerle zimmerle self-requested a review July 9, 2021 13:42
@877509395
Copy link

great for "delete transformations"

@@ -80,6 +80,10 @@ RuleWithActions::RuleWithActions(
m_containsStaticBlockAction(false),
m_isChained(false) {

if (transformations != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just understand. It's the same like delete actions do.

@poszu
Copy link

poszu commented Mar 28, 2022

@zimmerle What is holding you back from merging this? I also have a problem with these memory leaks, it would be great to merge it. ;)

@liudongmiao
Copy link
Contributor

@poszu From my test in #2710, even patched this, there are many memory leak.
Which g++ version do you use?

@poszu
Copy link

poszu commented Mar 28, 2022

@poszu From my test in #2710, even patched this, there are many memory leak. Which g++ version do you use?

Yes, I am aware of other leaks too. I'm using clang 10. ASAN reports plenty of leaks on the v3.0.6.

@@ -129,9 +129,9 @@ int Driver::parse(const std::string &f, const std::string &ref) {
m_lastRule = nullptr;
loc.push_back(new yy::location());
if (ref.empty()) {
loc.back()->begin.filename = loc.back()->end.filename = new std::string("<<reference missing or not informed>>");
loc.back()->begin.filename = loc.back()->end.filename = std::shared_ptr<const std::string>(new std::string("<<reference missing or not informed>>"));
Copy link

Choose a reason for hiding this comment

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

Consider using std::make_shared to avoid using new

Suggested change
loc.back()->begin.filename = loc.back()->end.filename = std::shared_ptr<const std::string>(new std::string("<<reference missing or not informed>>"));
loc.back()->begin.filename = loc.back()->end.filename = std::make_shared<const std::string>("<<reference missing or not informed>>");

@@ -129,9 +129,9 @@ int Driver::parse(const std::string &f, const std::string &ref) {
m_lastRule = nullptr;
loc.push_back(new yy::location());
if (ref.empty()) {
loc.back()->begin.filename = loc.back()->end.filename = new std::string("<<reference missing or not informed>>");
loc.back()->begin.filename = loc.back()->end.filename = std::shared_ptr<const std::string>(new std::string("<<reference missing or not informed>>"));
Copy link

Choose a reason for hiding this comment

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

Consider using std::make_shared to avoid using new

Suggested change
loc.back()->begin.filename = loc.back()->end.filename = std::shared_ptr<const std::string>(new std::string("<<reference missing or not informed>>"));
loc.back()->begin.filename = loc.back()->end.filename = std::make_shared<const std::string>("<<reference missing or not informed>>");

@martinhsv
Copy link
Contributor

martinhsv commented Sep 15, 2022

I don't believe the portions of this PR related to filename are correct.

The problem is that location.hh is a generated file. The manual change to that file suggested in this PR could be made and a build done. But thereafter, any time bison rebuilt the generated files, those manual changes would be lost.

I'll go ahead and commit the other two fixes via a separate PR:

  • Rule class was missing a virtual destructor
  • delete of transformations vector

The parser-filename issue can be addressed later via a separate issue or PR.

@martinhsv
Copy link
Contributor

The two issues have been merged via #2801 .

Thanks @Abce

@martinhsv martinhsv closed this Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants