Skip to content

fix stack overflow on nested JSON in JsonToInteger and fromJson - #1274

Open
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:json-nested-stack-overflow
Open

aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:json-nested-stack-overflow

Conversation

@aysha-afrah26

Copy link
Copy Markdown
Contributor

details::JsonToInteger (json_export.h) builds its error message with value.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 the uid and position fields of HOOK_INSERT, HOOK_REMOVE and BREAKPOINT_UNLOCK requests, with no authentication and bound to all interfaces by default, so one request kills the process through unbounded recursion (CWE-674). The try/catch in serverLoop doesn't help, since a stack overflow arrives as a signal and not as an exception. The json: prefix of a std::vector<int> port reaches the same line from the XML. The dump() 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::fromJson copied source[0] only to read its type (reachable through ImportBlackboardFromJSON and ImportTreeFromJSON), and loadSubstitutionRuleFromJSON copied both of its sections. Parsing and destruction are already safe at any depth, so no limit is needed: JsonToInteger now 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_INSERT on the 512 KB secondary thread stack: crashes from about 10 KB of request
  • dump() path on an 8 MB stack, the usual Linux thread default: from about 150 KB
  • copy paths on an 8 MB stack: from about 88 KB
  • with the fix: an 8 MB request with 4 million levels gets JSON value [array] is not an integer of type unsigned short and the server keeps answering

I 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.DeeplyNestedJsonDoesNotExhaustStack and Groot2PublisherIntegration.DeeplyNestedHookRequestDoesNotTerminateServer fail 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, so Groot2Test::Client gained an optional timeout_ms. The full suite passes in Release, ASan+UBSan and TSan except for two unrelated failures: LoggerTest.SqliteLogger_ExecSqlStatement crashes on most runs on untouched master too (9 of 12), and CoroTest.sequence_child failed once under TSan with -j6 and passed 8 of 8 on its own. clang-format 17.0.6 and codespell are clean. clangd-21 isn't available here, so I ran clang-tidy 21 directly, with no findings on the changed library lines.

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