Repository navigation
fix stack overflow on nested JSON in JsonToInteger and fromJson - #1274
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.
details::JsonToInteger(json_export.h) builds its error message withvalue.dump(). nlohmann's parser and destructor are iterative and accept any depth, but its serializer recurses once per nesting level, so a value like[[[[...]]]]where an integer is expected parses fine and then overflows the stack while the library composes the error. The Groot2 server runs this helper on theuidandpositionfields ofHOOK_INSERT,HOOK_REMOVEandBREAKPOINT_UNLOCKrequests, with no authentication and bound to all interfaces by default, so one request kills the process through unbounded recursion (CWE-674). Thetry/catchinserverLoopdoesn't help, since a stack overflow arrives as a signal and not as an exception. Thejson:prefix of astd::vector<int>port reaches the same line from the XML. Thedump()call was added in #1240.basic_json's copy constructor recurses per level too, and two sibling sites copied the same kind of data:JsonExporter::fromJsoncopiedsource[0]only to read its type (reachable throughImportBlackboardFromJSONandImportTreeFromJSON), andloadSubstitutionRuleFromJSONcopied both of its sections. Parsing and destruction are already safe at any depth, so no limit is needed:JsonToIntegernow names the type of an array or object instead of dumping it, and the copies become const references.Measured on a Release build without sanitizers on macOS arm64:
HOOK_INSERTon the 512 KB secondary thread stack: crashes from about 10 KB of requestdump()path on an 8 MB stack, the usual Linux thread default: from about 150 KBJSON value [array] is not an integer of type unsigned shortand the server keeps answeringI did not run this on Linux, so the 8 MB numbers come from the main thread on macOS.
API and ABI are unchanged: this touches an inline template and function bodies, no signature or layout. The one visible difference is the error text for an array or object, which now reads
[array]or[object]instead of the dumped value. Numbers and strings keep their message.JsonTest.DeeplyNestedValueDoesNotExhaustStack,Substitution.DeeplyNestedJsonDoesNotExhaustStackandGroot2PublisherIntegration.DeeplyNestedHookRequestDoesNotTerminateServerfail without the fix (ASan stack-overflow, SIGSEGV in Release) and pass with it. The Groot2 one needs a longer client timeout because parsing the request takes seconds under TSan, soGroot2Test::Clientgained an optionaltimeout_ms. The full suite passes in Release, ASan+UBSan and TSan except for two unrelated failures:LoggerTest.SqliteLogger_ExecSqlStatementcrashes on most runs on untouched master too (9 of 12), andCoroTest.sequence_childfailed once under TSan with-j6and passed 8 of 8 on its own. clang-format 17.0.6 and codespell are clean.clangd-21isn't available here, so I ran clang-tidy 21 directly, with no findings on the changed library lines.