Conversation
…rging msc_rules_add(), msc_rules_add_file(), msc_rules_add_remote() and msc_rules_merge() hand out a strdup()'ed error message and transfer its ownership to the caller. The connector never released it, so every failed config parse leaked the message. When the master process itself parses the broken config (a direct SIGHUP, as systemd's ExecReload does) the leak is retained for the lifetime of the master and grows with every failed reload. Add ngx_http_modsecurity_rules_error_free(), which uses the documented msc_rules_error_cleanup() from libmodsecurity v3.0.13 and falls back to free() on older releases, and call it in the three ngx_conf_set_rules* handlers and in ngx_http_modsecurity_merge_conf(). The connector's own strdup() of the message in the three handlers is left untouched on purpose: replacing it with a pool allocation is PR owasp-modsecurity#382's change. This commit only adds the release of the string libmodsecurity handed out, which owasp-modsecurity#382 does not do, and fixes merge_conf(), which owasp-modsecurity#382 does not touch. In merge_conf() the message is now copied into cf->pool before it is released. The copy is explicitly NUL-terminated because ngx_conf_handler() prints it with "%s". msc_rules_merge() is also wrapped in the pcre_malloc_init()/pcre_malloc_done() pair, like the three ngx_conf_set_rules* handlers already do. tests/modsecurity-config-error.t checks that nginx -t rejects an inline rules syntax error, a rules file syntax error and a rule id duplicated between a server and a location, and that the libmodsecurity message reaches the emerg line complete, with nginx's "in <conf>:<line>" suffix attached.
|
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 (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe connector now frees libmodsecurity rule errors after copying them, protects rule merging with PCRE allocation guards, and returns NUL-terminated nginx-pool copies. New tests validate invalid inline, file-based, and merged-rule configuration errors. ChangesConfiguration error handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 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 |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
const char **errorwhenmsc_rules_add(),msc_rules_add_file(),msc_rules_add_remote()ormsc_rules_merge()fail. New helperngx_http_modsecurity_rules_error_free()callsmsc_rules_error_cleanup()on libmodsecurity 3.0.13 and later (the same version guard the connector already uses formsc_set_request_hostname) andfree()on older releases, which is exactly what that API does.modsecurity_rules*handlers the existingstrdup(error)copy is taken first and libmodsecurity's string released afterwards. Thestrdupitself is deliberately left in place: fix: replace strdup(error) with nginx pool allocation in config handlers #382 replaces it with a pool allocation, and this change does not duplicate that.merge_conf()the message is copied intocf->poolwith an explicit NUL terminator (nginx prints it with%s;ngx_pstrdup()would not terminate it), libmodsecurity's string is released, andmsc_rules_merge()is wrapped in the samepcre_malloc_init/donepair the handlers use.tests/modsecurity-config-error.trunsnginx -ton three failing configurations (inline syntax error, rules-file syntax error, rule id duplicated between server and location) and checks the libmodsecurity message reaches the emerg line complete with nginx'sin <conf>:<line>suffix.why
merge_conf(), the connector's ownstrdupof it. These are per-configuration-parse leaks: they matter fornginx -tloops and for a master process that receives a bad configuration onSIGHUP(the leak repeats on every failed reload attempt; a-s reloadfrom a separate process does not accumulate because that process exits).strdup ← msc_rules_add*and bothmerge_confrecords are gone on both libmodsecurity 3.0.9 and 3.0.14; only the record fix: replace strdup(error) with nginx pool allocation in config handlers #382 fixes remains.references
strdup(error)in the three handlers to fix: replace strdup(error) with nginx pool allocation in config handlers #382. Note for that change:ngx_pstrdup()copieslenbytes without a terminator, whilengx_conf_handler()formats the returned message with%s; the copy needs an explicit NUL as done here formerge_conf().Origin: this fix comes from a memory-leak audit of the connector done with Claude Fable 5.1 (Anthropic), following its performance review. Verified by building against nginx master with libmodsecurity 3.0.14 (PCRE2) and with upstream CI's flags against libmodsecurity 3.0.9 (PCRE1), running the full
tests/modsecurity*.tsuite in both builds (16 files, 257 tests, all passing), and by valgrind runs ofnginx -ton each failing configuration before and after the change.Summary by CodeRabbit
Bug Fixes
Tests