Conversation
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughChangesRegex lifetime and validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When the parameter of @rx or @rxglobal contains a macro, the pattern has to be expanded and compiled on every evaluation, so Rx::evaluate() and RxGlobal::evaluate() allocate a temporary Utils::Regex with `new`. That object was only released by an explicit `delete` placed just before the final return statements, which the early `return false` paths never reach: * Rx::evaluate() returns early when the expanded pattern does not compile (re->hasError()) and when the match fails with a PCRE error such as MATCH_LIMIT. * RxGlobal::evaluate() returns early when the match fails with a PCRE error. Every request that hits one of those paths therefore leaks the Regex object together with the compiled pcre2 code and, when available, its JIT compiled counterpart. The operators are evaluated at least once per request and per target variable, so the leak grows without bound in a long running process. The raw pointer is replaced by a std::unique_ptr<Regex> that owns the macro expanded regex for the whole scope of evaluate(), and the manual delete is removed. The non macro case still uses the pre-compiled m_re owned by the operator, which is not freed by the unique_ptr. Two regression tests are added to operator-rx.json, one per early return in Rx::evaluate(): a macro expanding to a non-compiling pattern and a macro pattern that exceeds SecPcreMatchLimit. Running that file under valgrind reports 346 (96 direct, 250 indirect) bytes in 2 blocks are definitely lost at operator new(unsigned long) by modsecurity::operators::Rx::evaluate(...) (rx.cc:49) before the change and "All heap blocks were freed -- no leaks are possible" after it. No regression test is added for RxGlobal::evaluate(). With PCRE2, which is the default, Regex::searchGlobal() always returns RegexResult::Ok, so that early return is only reachable in --with-pcre (PCRE1) builds; the leak is fixed there all the same.
fccee89 to
5e1425f
Compare
|
1 participant
what
Rx::evaluate()andRxGlobal::evaluate()hold the macro-expanded, per-evaluationUtils::Regexin astd::unique_ptrfor the whole scope instead of a rawnew/deletepair placed before the final returns.operator-rx.json: a macro expanding to a non-compiling pattern, and a macro pattern that exceedsSecPcreMatchLimit.why
@rx/@rxGlobalparameter contains a macro, the pattern is expanded and compiled on every evaluation. The compiledRegexwas only deleted right before the finalreturn, which the earlyreturn falsepaths never reach:re->hasError()(expanded pattern does not compile) andregex_result != Ok(PCRE error such asMATCH_LIMIT;modsecurity.conf-recommendedsetsSecPcreMatchLimit 1000, so this is reachable with ordinary custom rules).Regexwith its compiled pcre2 code and JIT block, once per target variable per request, so a long-running worker grows without bound.RxGlobalhalf: with PCRE2Regex::searchGlobal()always returnsOk, so that early return is only reachable in--with-pcrebuilds; the change fixes it there too.Evidence, unfixed tree,
valgrind --leak-check=full ./.libs/regression_tests test-cases/regression/operator-rx.json:With the fix:
All heap blocks were freed -- no leaks are possible;operator-rx.json8/8, whole regression directory 722 passed / 11 skipped.Side note for a separate change:
RxGlobal::evaluate()has nohasError()check, so a macro that expands to an invalid pattern reachesRegex::searchGlobal()with a nullpcre2_code. Not touched here.references
Summary by CodeRabbit
Bug Fixes
Tests