Skip to content

The pool pointer is now available for ngx_http_modsecurity_config_cleanup #87

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

Closed

Conversation

AirisX
Copy link
Contributor

@AirisX AirisX commented Dec 22, 2017

I did a little research and found out that the rules that are initialized in the pool by the msc_rules_add_file or msc_rules_add can not be freed by the msc_rules_cleanup because ngx_http_modsecurity_config_cleanup does not know anything about this pool (since it is NULL).

@AirisX
Copy link
Contributor Author

AirisX commented Dec 25, 2017

It seems that this PR is also reasonable for owasp-modsecurity/ModSecurity#1546

@zimmerle zimmerle requested review from zimmerle and defanator March 26, 2018 19:37
@zimmerle zimmerle self-assigned this Mar 26, 2018
@@ -649,7 +650,7 @@ ngx_http_modsecurity_config_cleanup(void *data)

dd("deleting a loc conf -- RuleSet is: \"%p\"", t->rules_set);

old_pool = ngx_http_modsecurity_pcre_malloc_init(NULL);
old_pool = ngx_http_modsecurity_pcre_malloc_init(t->pool);
Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless the parameter here I guess the end result will be the same.

Copy link
Contributor

@zimmerle zimmerle Mar 26, 2018

Choose a reason for hiding this comment

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

oh wait, i think i've get your point. let me discuss with @defanator.

Copy link
Collaborator

@defanator defanator left a comment

Choose a reason for hiding this comment

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

@AirisX @zimmerle sorry for the delay with this one - I'm finally fine with merging it as is (note that the patch does not apply cleanly on the current master, though the fix is easy).

Thanks @AirisX!

@AirisX AirisX force-pushed the fix/conf_cleanup_pool branch from 7efd4f0 to c6ea19d Compare April 28, 2018 10:18
@AirisX
Copy link
Contributor Author

AirisX commented Apr 28, 2018

@defanator @zimmerle I did a rebase to apply patch more cleanly.

zimmerle pushed a commit that referenced this pull request May 3, 2018
@zimmerle
Copy link
Contributor

zimmerle commented May 3, 2018

Merged! Thanks!

@zimmerle zimmerle closed this May 3, 2018
pracj3am pushed a commit to cdn77/ModSecurity-nginx that referenced this pull request Nov 4, 2022
pracj3am added a commit to cdn77/ModSecurity-nginx that referenced this pull request Oct 10, 2023
pracj3am added a commit to cdn77/ModSecurity-nginx that referenced this pull request Oct 10, 2023
pracj3am added a commit to cdn77/ModSecurity-nginx that referenced this pull request Oct 10, 2023
pracj3am added a commit to cdn77/ModSecurity-nginx that referenced this pull request Oct 11, 2023
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.

3 participants