Skip to content

Bad performance when used with threaded mpms and a libapr compiled with "--enable-allocator-uses-mmap" option #768

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

Open
zimmerle opened this issue Aug 18, 2014 · 6 comments
Assignees
Milestone

Comments

@zimmerle
Copy link
Contributor

Originally reported by: Nelson Elhage and Stefan Fritsch.

It seems that ModSecurity is having bad performance whenever used with threaded mpms and a libapr compiled with "--enable-allocator-uses-mmap" option.

According to Stefan that option is used on apr's Debian package for over 3 years.

It is somehow related to the way that ModSecurity is individually handling every connection, by created a new memory allocator.

Relevant piece of code:

Original report:

@s-fritsch
Copy link

I would recommend trying this patch (compiles but otherwise untested). [BTW, is there a function to attach patches to issues? I couldn't find it.]

The patch causes the new pool to use the allocator (and it's pool of free memory) from the request pool. This should avoid calls to malloc/mmap for the memory. It also makes the pool obey the MaxMemFree setting from httpd's config, instead of using a hard coded value of 1024. The apr_allocator_max_free_set() here was mostly useless anyway, because the allocator was destroyed after the request and then the memory would be freed with free()/munmap().

--- a/apache2/mod_security2.c
+++ b/apache2/mod_security2.c
@@ -438,17 +438,13 @@ static void store_tx_context(modsec_rec *msr, request_rec *r) {
  * Creates a new transaction context.
  */
 static modsec_rec *create_tx_context(request_rec *r) {
-    apr_allocator_t *allocator = NULL;
     modsec_rec *msr = NULL;

     msr = (modsec_rec *)apr_pcalloc(r->pool, sizeof(modsec_rec));
     if (msr == NULL) return NULL;

-    apr_allocator_create(&allocator);
-    apr_allocator_max_free_set(allocator, 1024);
-    apr_pool_create_ex(&msr->mp, r->pool, NULL, allocator);
+    apr_pool_create(&msr->mp, r->pool);
     if (msr->mp == NULL) return NULL;
-    apr_allocator_owner_set(allocator, msr->mp);

     msr->modsecurity = modsecurity;
     msr->r = r;

While looking through the source I also saw that you create a pool without parent in modsecurity_request_body_start(). This causes the pool to use the global allocator. I would recommend using the some parent pool that is tied to the request pool. On threaded MPMs, this makes the new pool use an allocator that is local to the current thread, which should cause less mutex contention than accessing the global allocator from all threads:

--- a/apache2/msc_reqbody.c
+++ b/apache2/msc_reqbody.c
@@ -88,7 +88,7 @@ apr_status_t modsecurity_request_body_start(modsec_rec *msr, char **error_msg) {
      * to allocate structures from (not data, which is allocated
      * via malloc).
      */
-    apr_pool_create(&msr->msc_reqbody_mp, NULL);
+    apr_pool_create(&msr->msc_reqbody_mp, msr->mp);

     /* Initialise request body processors, if any. */

@sathieu
Copy link

sathieu commented Jun 29, 2016

@s-fritsch Can you provide a pull-request?

@marcstern
Copy link

This crashes httpd 2.4.23 on Windows

@victorhora victorhora added this to the v2.9.3 milestone Sep 22, 2018
@victorhora
Copy link
Contributor

The global mutex is optional since 112ba45 (included in 2.9.2 release).

This shouldn't be a concern with libModSecurity. libModSecurity (aka v3) is already officially released: https://github.com/SpiderLabs/ModSecurity/releases/tag/v3.0.2 (3.0.3 upcoming)

Thanks!

@shallot
Copy link

shallot commented Feb 1, 2019

It's not quite clear how this is related to the global mutex from #1224

The fixed allocator mentioned in the original reports is still around https://github.com/SpiderLabs/ModSecurity/blob/v2/master/apache2/mod_security2.c#L481

@victorhora
Copy link
Contributor

hummm indeed @shallot. My bad. It was closed by mistake when looking for issues related with the mutex.

Reopening and tagging for review prior to 2.9.4 release.

If you would like to help the community by testing the proposed patch and reporting back it would be great too!

Thanks!

@victorhora victorhora reopened this Feb 2, 2019
@victorhora victorhora modified the milestones: v2.9.3, v2.9.4 Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants