Fix rules-loading memory leaks on SecDefaultAction error paths and repeated msg/severity/logdata - #3638
Open
tomsommer wants to merge 2 commits into
Open
Fix rules-loading memory leaks on SecDefaultAction error paths and repeated msg/severity/logdata#3638tomsommer wants to merge 2 commits into
tomsommer wants to merge 2 commits into
Conversation
added 2 commits
September 19, 2026 16:27
The SecDefaultAction rule released the parsed actions out of the bison semantic value into a raw std::vector<actions::Action *> allocated with new, and only deleted that vector on the last line of the action. All four validation failures in between (transformation none, action not suitable for SecDefaultAction, missing disruptive action, phase already configured) leave the block through YYERROR, so the vector and every Action it had taken ownership of were leaked. A configuration error is not a one-off: nginx re-parses the whole rule set in the master process on every reload and on every configuration test, so a single bad SecDefaultAction leaks on each of them. Instead of releasing the actions, iterate over the semantic value by reference and move the suitable ones into a local std::vector<std::unique_ptr<actions::Action>>, which is then moved into driver.m_defaultActions. The phase action no longer needs an explicit delete, it is simply left behind and destroyed with the semantic value. YYERROR is a goto, so the destructors of the block scope locals still run, and bison reclaims the rule's right hand side with yypop_() in yyerrorlab, which destroys the variant holding the vector of unique_ptr. Nothing is leaked on either path. The regenerated seclang-parser.cc is included; it was produced with the same bison 3.8.2 that generated the checked-in file, so the diff is limited to the rule body, the yyrline_ table and the #line directives. A regression test covering a SecDefaultAction without a disruptive action was added to test/test-cases/regression/config-secdefaultaction.json.
RuleWithActions keeps a single pointer for each of the msg, logdata and
severity actions and deletes them in its destructor, but the constructor
assigned over whatever was already stored there. A rule that names one of
those actions more than once, for example
SecRule ARGS "@contains x" "id:1,phase:2,pass,msg:'a',msg:'b'"
therefore leaked one Action (and the RunTimeString it owns) per rule, per
parse. Rule sets are re-parsed on every nginx reload and on every nginx
configuration test, so the loss accumulates in the master process.
Delete the previous action before storing the new one. The last occurrence
still wins, which is the behaviour that was already in place.
|
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 (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe parser now uses ChangesAction memory management
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low 🚥 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 1 functions across 1 files. (3 skipped: 3 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
Two commits, one fix each:
SecDefaultAction: the actions are no longer released into a rawstd::vector<Action *>*; they stay inunique_ptrs (the$2vector and astd::vector<std::unique_ptr<Action>>for the accepted ones) and are moved intodriver.m_defaultActionsat the end. The regeneratedseclang-parser.ccis included (Bison 3.8.2, same as the checked-in file; regenerating the unmodified grammar reproduces it byte for byte, so the diff is only the rule body plus#linerenumbering).RuleWithActionsconstructor:deletethe previousm_severity/m_logData/m_msgbefore overwriting it when a rule repeatsseverity:,logdata:ormsg:(last one still wins).config-secdefaultaction.jsonwith the expected parser error, and a "msg informed twice, last one wins" case inaction-msg.json.why
YYERRORexits of theSecDefaultActionrule (t:none, unsuitable action, no disruptive action, duplicate phase) jumped out beforedelete actions, leaking the vector and every released action. Only invalid configurations hit this, but under nginx a repeatedly failingnginx -s reloadornginx -taccumulates it in the master. Withunique_ptrs, both thegotoout of the block and Bison's pop of$2during error recovery destroy everything.msg:(orseverity:/logdata:) actions leaked oneActionper parse; the members are null-initialised in the constructor and deleted in the destructor, so deleting before overwrite is safe.Evidence, unfixed tree:
With the fixes both files report
All heap blocks were freed -- no leaks are possible.make checkon this branch: TOTAL 5045, PASS 5029, SKIP 16, FAIL 0.references
Summary by CodeRabbit
msgactions so the final message is consistently retained and reported.SecDefaultActionconfigurations without a disruptive action continue to be rejected with a clear validation error.