Skip to content

Conversation

@facontidavide
Copy link
Collaborator

@facontidavide facontidavide commented Feb 2, 2026

Summary

  • Added depth tracking to recursive XML parsing in VerifyXML() and recursivelyCreateSubtree()
  • Throws RuntimeError when XML nesting exceeds 256 levels, preventing stack overflow crashes
  • Added 11 new tests covering malformed XML: deeply-nested trees, missing required attributes, unknown node types, empty trees, duplicate IDs, and other edge cases

Test plan

  • Existing tests pass (331/331, excluding pre-existing flaky PostConditions.Issue601 timer race)
  • New BehaviorTreeFactory.MalformedXML_* tests verify depth limit and other malformed XML rejection
  • Verified that valid deep trees (within limit) still work

Closes #672

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Added protection against excessively deep XML nesting to prevent stack overflow conditions.
    • Improved error handling for malformed XML inputs with descriptive error messages.
    • Enhanced validation for empty or invalid behavior tree definitions.

…ack overflow

Adds a depth limit (256) to the recursive XML parsing functions in
VerifyXML() and recursivelyCreateSubtree(). Malformed XML with extreme
nesting now throws a clear RuntimeError instead of crashing.

Includes 11 new tests for malformed XML handling.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 2, 2026

📝 Walkthrough

Walkthrough

The changes add XML nesting depth validation to prevent stack-buffer-overflow vulnerabilities. A maximum nesting depth limit of 256 is enforced in the XML parsing recursion, with depth parameters propagated through internal recursive functions and error handling when the limit is exceeded. Corresponding regression tests validate depth enforcement and malformed XML scenarios.

Changes

Cohort / File(s) Summary
XML Nesting Depth Validation
src/xml_parsing.cpp
Introduces kMaxNestingDepth constant (256) and propagates depth parameter through internal recursive lambdas (recursiveStep, recursivelyCreateSubtree). Adds depth guards that throw descriptive errors when nesting exceeds limit. Updates initial recursive invocations to start with depth 0.
Regression Tests for XML Validation
tests/gtest_factory.cpp
Adds comprehensive test suite covering invalid/deeply nested XML inputs: invalid root, missing root element, empty BehaviorTree, missing IDs with multiple trees, excessive nesting (expects RuntimeError), moderate nesting (expects success), multiple children under BehaviorTree, empty input, and unknown node types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #1053: Also modifies src/xml_parsing.cpp and recursivelyCreateSubtree, adding duplicate SubTree path validation alongside depth checks.

Poem

🐰 A rabbit hops through XML trees,
Counting nestings with such ease—
At depth 256, we say "no more!"
Stack overflow fears are what we swore
To guard against with patient care. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically references issue #672 and describes the main change: preventing stack overflow from deeply-nested XML.
Description check ✅ Passed The description covers all essential aspects: summary of changes, depth limit implementation, comprehensive test coverage, test results, and issue closure.
Linked Issues check ✅ Passed The PR fully addresses issue #672 by implementing depth tracking in XML parsing, throwing explicit errors when nesting exceeds 256 levels, and providing comprehensive regression tests for malformed XML.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing issue #672: depth tracking in xml_parsing.cpp and regression tests in gtest_factory.cpp for XML parsing robustness.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/issue-672

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Feb 2, 2026

Codecov Report

❌ Patch coverage is 95.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.75%. Comparing base (2e72fb3) to head (3b01094).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/xml_parsing.cpp 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   66.59%   66.75%   +0.15%     
==========================================
  Files         225      225              
  Lines       12853    12907      +54     
  Branches     1197     1205       +8     
==========================================
+ Hits         8560     8616      +56     
+ Misses       4243     4241       -2     
  Partials       50       50              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@facontidavide facontidavide merged commit f9a6f98 into master Feb 2, 2026
15 checks passed
@facontidavide facontidavide deleted the fix/issue-672 branch February 2, 2026 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stack-buffer-overflow in xml_parsing.cpp

1 participant