Repository navigation
fix nullptr dereference in script assignment of an unset entry - #1265
Open
aysha-afrah26 wants to merge 1 commit into
Open
aysha-afrah26 wants to merge 1 commit into
aysha-afrah26 wants to merge 1 commit into
Conversation
This branch has not been deployed
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.
ExprAssignment::evaluateresolves the target entry withgetEntry(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 throughBlackboard::entryInfo(key)and calledconverter()on the result without checking it. If the key is unset between the two lookups (anUnsetBlackboardnode orBlackboard::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 onlystorage_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 executingx = '42'in a loop while another didunset("x")/set<int>("x", 1), and ASan reported the SEGV inoperators.hppon each of three runs. Readingentry->infofrom the entry that is already held and locked fixes it, which is also what theAnyTypeAllowedcheck a few lines above does. The change is header-inline with no API or ABI impact, andParserTest.AssignStringWhileEntryIsUnsetopens the window deterministically through a subscriber: it crashes on master and passes with the fix. The full ctest run is green apart fromLoggerTest.SqliteLogger_ExecSqlStatement, which crashes the same way on master.