Repository navigation
XMLParser: detect cyclic <include> instead of overflowing the stack - #1250
Merged
Merged
Conversation
A tree file that includes itself, directly or through other files, made loadDocImpl() recurse until the process crashed with a stack overflow (SIGSEGV), because the include loop never checked whether a file was already being loaded. Track the normalized paths of the files in the active include chain and throw a RuntimeError naming the repeated file. The chain is a stack (pushed before, popped after each include), so a file included twice from different branches is not reported as a cycle; it is reset by the top-level loadFromFile()/loadFromText(), so a failed load leaves no stale state in the factory's parser. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
facontidavide
added a commit
to KImanalieva/BehaviorTree.CPP
that referenced
this pull request
Oct 24, 2026
Registering a BehaviorTree ID that already exists replaced the previous definition silently. Making it an error breaks legitimate uses of the deliberate last-wins behavior (a851c79): registering the same file twice, calling createTreeFromText repeatedly on one factory, diamond includes, and replacing a subtree with a test double. Keep last-wins and print a warning instead, only when the previous definition came from a different source or from the same document; reloading a file, including it from two places, or reusing the same inline text stays silent. Take the source from the include stack added by BehaviorTree#1250 instead of a second current_file variable, document the behavior in bt_factory.h, revert the clearRegisteredBehaviorTrees() calls added to existing tests, and turn the new tests into warning/last-wins checks, plus diamond-include and repeated-inline-text cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Problem
A tree XML whose
<include>resolves back to itself — directly (a.xmlincludesa.xml) or through other files (a.xml→child/b.xml→../a.xml) — makesXMLParser::PImpl::loadDocImpl()recurse until the stack overflows. The process dies with SIGSEGV instead of getting a load error. The include loop runs before any<BehaviorTree>is registered, so no other check ever stops it.Fix
PImpl::include_stackholds the normalized (weakly_canonical) paths of the files in the active include chain.loadFromFile()seeds it with the root file.<include>checks the stack. If the file is already in it, aRuntimeErrornames the file:Cyclic <include> detected: the file [...] includes itself, directly or through other files.loadFromFile()/loadFromText(). The factory reuses one parser, and this keeps a failed load from leaving stale entries behind.The change is confined to the PImpl in
xml_parsing.cpp, so there's no API/ABI change. Behavior change: cyclic includes used to crash the process and now throwRuntimeError.Tests
CreateTreeFromFileWhichIncludesItself: crashes with exit 139 (SIGSEGV) on master, passes with the fix.CreateTreeFromFileWithCyclicIncludes: a two-file cycle across directories (../), followed by a successful load on the same factory to prove no state is left over.🤖 Generated with Claude Code