From f732fc61e45a026164b635175d3386ec4885f37a Mon Sep 17 00:00:00 2001 From: asterite Date: Thu, 6 Feb 2020 11:51:13 +0300 Subject: [PATCH 1/2] Allocate regex in request's mempoll in @rx execute If regex contains variable substitution, function msre_op_rx_execute(), which handles execution of @rx operator, will compile regex on each request (because variable, and this regex, can be different each time). It does so by calling msc_pregcomp_ex(), which accepts a mem pool because it does allocations for regex data. Until this fix, msre_op_rx_execute() passed rule->ruleset->mp to msc_pregcomp_ex(), which is a ruleset mempool that persists as long as ruleset lives - which means it is long-living and will persist until ModSecurity is stopped/restarted. This created a memory leak because regex data for non-constand regexes was allocated again to each request and not freed after request was processed. Fix that by passing msr->mp mempool to msc_pregcomp_ex() instead, which is a mempool of record corresponding to a tx being processed and will be freed as soon as processing is done. --- apache2/re_operators.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apache2/re_operators.c b/apache2/re_operators.c index b639ae4f6e..a5ccbe589e 100644 --- a/apache2/re_operators.c +++ b/apache2/re_operators.c @@ -1018,7 +1018,7 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c msr_log(msr, 6, "Escaping pattern [%s]",pattern); } - regex = msc_pregcomp_ex(rule->ruleset->mp, pattern, PCRE_DOTALL | PCRE_DOLLAR_ENDONLY, &errptr, &erroffset, msc_pcre_match_limit, msc_pcre_match_limit_recursion); + regex = msc_pregcomp_ex(msr->mp, pattern, PCRE_DOTALL | PCRE_DOLLAR_ENDONLY, &errptr, &erroffset, msc_pcre_match_limit, msc_pcre_match_limit_recursion); if (regex == NULL) { *error_msg = apr_psprintf(rule->ruleset->mp, "Error compiling pattern (offset %d): %s", erroffset, errptr); From 5859417502d3058599c0c84261db5a8fe886c7c1 Mon Sep 17 00:00:00 2001 From: asterite Date: Thu, 6 Feb 2020 12:03:27 +0300 Subject: [PATCH 2/2] Use right PCRE functions to alloc/free pcre_extra 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 #610 --- apache2/msc_pcre.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/apache2/msc_pcre.c b/apache2/msc_pcre.c index 8534a20914..989a7f7ead 100644 --- a/apache2/msc_pcre.c +++ b/apache2/msc_pcre.c @@ -21,11 +21,7 @@ static apr_status_t msc_pcre_cleanup(msc_regex_t *regex) { if (regex != NULL) { if (regex->pe != NULL) { -#if defined(VERSION_NGINX) - pcre_free(regex->pe); -#else - free(regex->pe); -#endif + pcre_free_study(regex->pe); regex->pe = NULL; } if (regex->re != NULL) { @@ -67,19 +63,15 @@ void *msc_pregcomp_ex(apr_pool_t *pool, const char *pattern, int options, #ifdef WITH_PCRE_STUDY #ifdef WITH_PCRE_JIT - pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr); + pe = pcre_study(regex->re, PCRE_STUDY_EXTRA_NEEDED|PCRE_STUDY_JIT_COMPILE, &errptr); #else - pe = pcre_study(regex->re, 0, &errptr); + pe = pcre_study(regex->re, PCRE_STUDY_EXTRA_NEEDED, &errptr); #endif #endif /* Setup the pcre_extra record if pcre_study did not already do it */ if (pe == NULL) { -#if defined(VERSION_NGINX) pe = pcre_malloc(sizeof(pcre_extra)); -#else - pe = malloc(sizeof(pcre_extra)); -#endif if (pe == NULL) { return NULL; }