Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Crashes in XML Verification Due to Invalid Node Name Processing #918

Closed
cktii opened this issue Jan 31, 2025 · 1 comment
Closed
Assignees
Labels
bug Something isn't working

Comments

@cktii
Copy link

cktii commented Jan 31, 2025

Description

Two related issues in BehaviorTreeFactory's XML verification can lead to crashes when processing XML nodes:

  1. Incorrect Child Name Access: The code always gets the name of the first child instead of the current child in the loop
  2. Invalid Iterator Access: The code attempts to dereference an iterator without checking if the node type was found

size_t async_count = 0;
for(auto child = node->FirstChildElement(); child != nullptr;
child = child->NextSiblingElement())
{
const std::string child_name = node->FirstChildElement()->Name();
const auto child_search = registered_nodes.find(child_name);
const auto child_type = child_search->second;

Found in commit: 48f6c5b

Bug Class

Memory Safety - Null Pointer Dereference

Crash Details

The crash occurs in VerifyXML() at line 553:

const std::string child_name = node->FirstChildElement()->Name();  // Wrong!
const auto child_search = registered_nodes.find(child_name);
const auto child_type = child_search->second;  // Crashes here

Root Cause

In the loop processing children, the code incorrectly uses node->FirstChildElement() instead of child->Name(). When an unknown node type (":eq") is encountered, the code doesn't check if it was found in registered_nodes before accessing.

Impact

  • Memory access violation/segfault when processing XML with unknown node types
  • Logic error: validation only checks the first child repeatedly, missing issues with other children

Proposed Fix

for(auto child = node->FirstChildElement(); child != nullptr; child = child->NextSiblingElement())
{
    const std::string child_name = child->Name();  // Fix 1: Use current child
    const auto child_search = registered_nodes.find(child_name);
    if(child_search == registered_nodes.end()) {   // Fix 2: Check iterator
        ThrowError(child->GetLineNum(), 
                  std::string("Unknown node type: ") + child_name);
    }
    const auto child_type = child_search->second;
    // ... rest of the code ...
}

Additional Notes

  • Found via fuzzing with AFL++ and AddressSanitizer
  • The bug affects XML validation, which is a critical security boundary for the library
  • Both issues appear in the same code block but are independent problems
@facontidavide facontidavide self-assigned this Jan 31, 2025
@facontidavide facontidavide added the bug Something isn't working label Jan 31, 2025
facontidavide added a commit that referenced this issue Feb 9, 2025
@facontidavide
Copy link
Collaborator

I believe this was introduced in #885. Pinging @AndyZe

Thanks for the fix. i am pushing it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants