Memory leak if builded modsecurity with --enable-pcre-study #610
Comments
|
Yes, this is a leak. In my case (ModSecurity under nginx) memory was ok (no leak) under Ubuntu 14.04, but when using under Ubuntu 16.04 - nginx would leak memory with every reload. Applying this change fixed it. Thank you @weliu. |
|
Glad it helped. @kipras |
|
Adding the following code, right before the "if (regex->pe != NULL)" block, should do the work: Correct? |
|
I think the '#ifdef WITH_PCRE_JIT' should be removed. Only insert below code before "if (regex->pe != NULL)" block: #ifdef WITH_PCRE_STUDY Moreover, I think we should check the return value of pcre_study() in msc_pregcomp_ex(): |
|
In case WITH_PCRE_JIT is not defined, pe is allocated with pcre_malloc() or malloc(). Additional question: why using malloc() instead of pcre_malloc() under Apache? |
|
Here's how the fixed block looks like in my case: The previous unpatched version that leaked memory was: Although, now that i look at it, it should probably be Or is Checking |
|
Checking pe after pcre_study() call is definitely a good idea |
|
kipras's modification looks more reasonable to me. |
|
One thing worth mentioning is that the --enable-pcre-study configuration flag seems to be enabled by default on v2 since ModSec 2.5. The changelog entry from 2.5.12 also suggests it's the case. My tests with Valgring suggests that the proposed patches actually makes the memory leak more noticeable. My test environment was the following: ModSec flags: Apache/ModSec Configuration: Request generated with Apache Benchmark: Memory check: This is a snippet of the leak summaries I got when running Apache Benchmark + CRS with the patch applied: And the same run without the patch: Am I doing something wrong? |
Function msc_pcre_cleanup(), which is responsible for freeing compiled regex data, used either regular free() or pcre_free() (depending on compilation settings) to free pcre_extra structure (pointer to which is stored in regex->pe) created by pcre_study(). This was incorrect, structure returned by pcre_study() should be freed by function pcre_free_study(). In case PCRE JIT is used, pcre_study() makes some additional allocations itself (at least for JITed executable code), which function pcre_free_study() frees. If pcre_free_study() is not used a memory leak occurs because, while pcre_extra structure itself might be freed by regular free(), some additional data referenced by it is not. Fix that by calling pcre_free_study() (instead of free()/pcre_free()) on pointer returned by pcre_study(). Note that code creating msc_regex_t may allocate regex->pe with malloc() or pcre_malloc() instead of pcre_study(). This case is checked by testing if PCRE_EXTRA_EXECUTABLE_JIT flag on regex->pe->flags is set. msc_pregcomp_ex() does not set that flag itself (and it memsets the whole structure with zeros after allocation) and pcre_free_study() actually does the same (it tests for PCRE_EXTRA_EXECUTABLE_JIT flag, and, if that is zero, calls pcre_free() on passed pointer). Fixes owasp-modsecurity#610
Allocate pcre_extra structure with pcre_study or pcre_malloc and free it with pcre_free_study(). Pass flag PCRE_STUDY_EXTRA_NEEDED to pcre_study telling it to always allocate pcre_extra because this structure is needed anyway to configure match limits. Until this change, pcre_extra was allocated with eigther pcre_malloc or regular malloc (depending on whether VERSION_NGINX is defined); function msc_pcre_cleanup(), which is responsible for freeing compiled regex data, used either regular free() or pcre_free() (depending VERSION_NGINX too) to free pcre_extra structure (pointer to which is stored in regex->pe). Freeing it like this was incorrect, structure returned by pcre_study() should be freed by function pcre_free_study(). In case PCRE JIT is used, pcre_study() makes some additional allocations itself (at least for JITed executable code), which function pcre_free_study() frees. If pcre_free_study() is not used a memory leak occurs because, while pcre_extra structure itself might be freed, some additional data referenced by it is not. Fix that by calling pcre_free_study() (instead of free()/pcre_free()) on pointer returned by pcre_study(). There also seems to be no reason to allocate pcre_extra with regular malloc (and de-allocate it with free()) - there is a function pcre_malloc(), which is a function pcre_study() itself would use to allocate that memory, and, in default case, pcre_malloc and pcre_free will be set to regular malloc and free. Usage of malloc() seems to be a remaining of old code where manual allocation of pcre_extra was always done with malloc(). So, remove "#if defined(VERSION_NGINX)" branches and always use pcre_malloc() for pcre_extra allocation in case pcre_study did not allocate it yet and always free is with pcre_free_study() (btw. 'pcreapi' man page recommends to replace pcre_free() usages to deallocate pcre_extra with pcre_free_study()). Fixes owasp-modsecurity#610
|
Closing due to no feedback. |
|
No feedback? |
|
Issue #2263 has a pretty write up and details the issue and how to reproduce. No need to keep this one. It will only fragment any discussion on the subject. |
|
Then it's because it's a duplicate, OK |
ghost
mentioned this issue
In apache2/msc_pcre.c line 68-74, we will get regex->pe from pcre_study() if we compile with --enable-pcre-study.
#ifdef WITH_PCRE_STUDY
#ifdef WITH_PCRE_JIT
pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr);
#else
pe = pcre_study(regex->re, 0, &errptr);
#endif
#endif
However we will use pcre_free() or free() to free it in msc_pcre_cleanup(), the right choice here is pcre_free_study().
if (regex->pe != NULL) {
if defined(VERSION_NGINX)
else
endif
This will lead to memory leak, the memory that leaked was allocated use SLJIT_MALLOC in pcre_study().
This can be a big problem if we use Apache graceful restart.
The text was updated successfully, but these errors were encountered: