Conversation
ValidateSchema::evaluate() installs ValidateSchema::error_load() and ValidateSchema::warn_load() as the libxml2 schema parser callbacks and passes the address of the ValidateSchema member m_err as their user context. Those callbacks append to that string and nothing ever clears it. An operator instance is created while the rules are parsed and lives for as long as the rule set does, which for a web server means the lifetime of the process. Therefore: * every evaluation whose schema parse emits a warning or an error makes m_err grow by the size of the new message, without any bound, and * the message logged for one transaction contains the messages of all the previous transactions that used the same rule, and * concurrent transactions sharing the rule set append to the same std::string at the same time, which is a data race. The accumulated text is per evaluation state, so it is moved to a local std::string in evaluate() and the member is removed from the header. The member was declared inside the WITH_LIBXML2 guard and is only used by the guarded code, so builds configured with --without-libxml are unaffected. The check of the accumulated text in the branch where the parser context itself cannot be created is dropped: at that point no callback has run yet, so the local is always empty. The unbounded part of the growth is cross request and cannot be observed by the single transaction regression harness, but the accumulation is visible within one transaction as soon as the same operator instance is evaluated twice. The regression test added to request-body-parser-xml.json therefore reuses the existing SoapEnvelope-bad.xsd case, which fails to parse, and adds multiMatch so that the operator runs once per transformation. Before this change the debug log of the second evaluation reads XML: Failed to load Schema: test-cases/data/SoapEnvelope-bad.xsd. XML Error: Failed to parse the XML resource '...'. XML Error: Failed to parse the XML resource '...'. with the message repeated once per previous evaluation; after it every evaluation logs the message exactly once. The expected debug_log of the new test asserts that no log line starts with a repeated "XML Error:", so it fails before the change and passes after it.
|
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)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe schema validation operator now stores parser errors in a local string instead of a persistent member. Failure logs use the local string. A regression test checks repeated bad-schema evaluations. ChangesSchema Error Isolation
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 1 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
ValidateSchema::evaluate()passes a localstd::stringas the libxml2 schema-parser error/warning context instead of the operator memberm_err; the member is removed.request-body-parser-xml.json: the existing bad-schema case withmultiMatch, so the same operator instance is evaluated twice in one transaction; it asserts the error text is not repeated. It fails before the change and passes after it.why
error_load()/warn_load()append into their context and nothing ever clearedm_err. An operator instance lives as long as the rule set, i.e. the process lifetime in nginx, and the schema is re-parsed on every evaluation. So every evaluation whose schema parse emits a warning or error grew the string without bound, the logged message for one transaction contained the messages of all previous ones, and concurrent transactions appended to the same string, which is a data race.WITH_LIBXML2guard and had no other reader;--without-libxmlcompilation of the translation unit was verified.Evidence (unfixed tree, new test): the second evaluation logs
with the message repeated once per previous evaluation. After the fix each evaluation logs it once. This is growth of a live string, so valgrind reports no lost blocks; the test is the evidence.
make checkon this branch: TOTAL 5044, PASS 5028, SKIP 16, FAIL 0.references
Summary by CodeRabbit
Bug Fixes
Tests