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

fixed memory leak on nginx reload #815

Closed

Conversation

lneto
Copy link

@lneto lneto commented Dec 16, 2014

  • created a root pool to destroy modsec pool safely among reloads

Felipe Zimmerle and others added 11 commits July 30, 2014 14:36
Refactoring on the nginx module, including:
 - Better handling larger posts;
 - Now using nginx echo module during the regression tests.
 - Better interacting with neginx chain rules
 - Separation of the request handling and content filters.
 - Better handling nginx sessions and resource counts to allow a
   more efficient garbage collector.
 - Handling both http/1.0 and 1.1, including keep-alive.
 - Tests are now capable to test nginx as a proxy or end-server.
 - Tested agains nginx 1.6 and 1.7.
If nginx segfaults it will return, warning that the test failed.
Add a check for the definition MOVE_REQUEST_CHAIN_TO_MODSEC, whenever it is
set the chain will be moved into the brigade. If it was not set the chain
will be only copied. Moving was causing segfaults on the following
regression tests:

 owasp-modsecurity#15 - SecRequestBodyInMemoryLimit
 owasp-modsecurity#16 - SecRequestBodyInMemoryLimit (greater)
 owasp-modsecurity#19 - SecRequestBodyLimitAction ProcessPartial (multipart/greater - chunked)
 (from: regression/config/10-request-directives.t)
Otherwise nginx's installation directory could not be specified.

Signed-off-by: paulyang <[email protected]>
This eliminates segfaults caused by unset (NULL) r->port_start
and non-NULL r->port_end. In fact, r->port_start is always NULL,
so it is useless to rely on this pointer.
* created a root pool to destroy modsec pool safely among reloads
@lneto
Copy link
Author

lneto commented Jan 19, 2015

Please, don't merge yet. A new PR is comming ;-).

@zimmerle
Copy link
Contributor

Just to keep registered here, we have being discussing about those changes and @lneto is preparing some modifications on the patches that belongs to that Pull Request. I will close it as a new Pull Request is coming. Thank you @lneto.

@daniilyar-confyrm
Copy link

@zimmerle, I can't find the new pull request from @lneto you are referring to, so asking here:

We face the said memory leak with ModSec 2.9.0 compiled from code with Nginx 1.8.0. We are running Nginx+ModSec in pair with Consul-template, which reloads Nginx config anytime when backend service config changes in Consul. Our Nginx servers are running out or memory after 100-200 config reloads, which totally breaks the stability of our (pretty dynamic) infra.

So I am wondering why, after one year, fix is still not present in a stable branch.. What blocks the release of this fix?

I will be glad to help, e.g. to help with reproducing this issue by providing a Docker image with erroneous Nginx + Modsec with reproducer script.
I am not a C++ pro, but I can try to help with patching and testing existing stable codebase also..

@daniilyar-confyrm
Copy link

Oops, sorry, missed the opened #895, added my question there

@zimmerle
Copy link
Contributor

zimmerle commented Feb 2, 2016

Hi @daniilyar-confyrm,

As you may able to see by @lneto comments, he is preparing some modifications to send another pull request.

We are currently working in a new version of ModSecurity, version 3. This version 3 works in a different fashion fixing not only this issue, but other related to this nginx implementation.

There are further information about libModSecurity here: http://tinyurl.com/zdyhgxk

@lneto
Copy link
Author

lneto commented Feb 2, 2016

Actually, I was; but it turns out that I couldn't come out with a
proper fix. Sorry =/. I think libModSecurity is a better approach.

Regards,

Lourival Vieira Neto

On Tue, Feb 2, 2016 at 1:37 PM, Felipe Zimmerle
[email protected] wrote:

Hi @daniilyar-confyrm,

As you may able to see by @lneto comments, he is preparing some modifications to send another pull request.

We are currently working in a new version of ModSecurity, version 3. This version 3 works in a different fashion fixing not only this issue, but other related to this nginx implementation.

There are further information about libModSecurity here: http://tinyurl.com/zdyhgxk


Reply to this email directly or view it on GitHub.

@daniilyar-confyrm
Copy link

@zimmerle, @lneto, thank you for an update.

@zimmerle, are you going to merge at least critical fixes (like fixes for memory and file descriptor leaks) to 2.9 version or you are going to stop 2.9 version support?

So, what should current ModSec + Nginx users do until ModSec 3.0 will be ready for PROD usage: switch to Apache or wait for ModSec 2.9.1 with critical Nginx-related fixes merged into last stable code?

@zimmerle
Copy link
Contributor

zimmerle commented Feb 2, 2016

Hi @daniilyar-confyrm,

Currently all the critical issues are merged into the "nginx_refactoring" branch. This branch won't be released. The version 2.9.1-RC1 will be release tomorrow (if everything goes right), but with no updates to nginx version.

That particular fixes are not merged because they may lead to other problems, currently they are only affecting dynamic configuration such as yours. Thus, very restrict set of users.

Keep tuned on the libModSecurity updates, i think it seems promising to your infrastructure.

@daniilyar-confyrm
Copy link

Currently all the critical issues are merged into the "nginx_refactoring" branch

@zimmerle, using this branch at PROD is not acceptable for us because except for the said fix there is a lot of other changes not qualified by community..

So I forked the 2.9.0 version and applied only critical fixes from 'nginx_refactoring' branch on the top of it: https://github.com/daniilyar/ModSecurity.

This fork is tested under the load at our dynamic env and satisfies our needs. We are going to merge all farther critical fixes to it until new 'clear and shiny' ModSec 3.0 (http://blog.zimmerle.org/2016/01/an-overview-of-upcoming-libmodsecurity.html) will be at least near-to-PROD-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants