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

[zimmerle]

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:

• https://github.com/SpiderLabs/ModSecurity/blob/master/apache2/mod_security2.c#L447-L451

Original report:

• https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=758068
  Option "--enable-allocator-uses-mmap" was added on libapr by Stefan as of 1.4.3, ChangeLog:
• http://svn.apache.org/viewvc/apr/apr/branches/1.4.x/CHANGES?view=markup
  Mail sent to ModSecurity devs mailing list:
• http://sourceforge.net/p/mod-security/mailman/message/32723547/

[s-fritsch]

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]

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

[marcstern]

This crashes httpd 2.4.23 on Windows

[victorhora]

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]

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]

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!