Skip to content

fix nullptr dereference in script assignment of an unset entry - #1265

Open
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:script-assign-unset-entry
Open

aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:script-assign-unset-entry

Conversation

@aysha-afrah26

Copy link
Copy Markdown
Contributor

ExprAssignment::evaluate resolves the target entry with getEntry(key) before evaluating the right-hand side, and then locks it. When a string is assigned to an entry of another type, it looked the key up a second time through Blackboard::entryInfo(key) and called converter() on the result without checking it. If the key is unset between the two lookups (an UnsetBlackboard node or Blackboard::unset() on another thread sharing the blackboard, or an entry-update subscriber reacting to a write made by the right-hand side), entryInfo() returns nullptr and the process crashes with SIGSEGV. If the key is unset and created again in that window, the pointer refers to a different entry that only storage_ keeps alive, so that entry's converter is applied to the old one and a further unset can leave the pointer dangling. I ran into it with one thread executing x = '42' in a loop while another did unset("x") / set<int>("x", 1), and ASan reported the SEGV in operators.hpp on each of three runs. Reading entry->info from the entry that is already held and locked fixes it, which is also what the AnyTypeAllowed check a few lines above does. The change is header-inline with no API or ABI impact, and ParserTest.AssignStringWhileEntryIsUnset opens the window deterministically through a subscriber: it crashes on master and passes with the fix. The full ctest run is green apart from LoggerTest.SqliteLogger_ExecSqlStatement, which crashes the same way on master.

This branch has not been deployed

No deployments
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.

1 participant