Skip to content

reject non-integer JSON numbers where an integer is expected - #1240

Open
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:json-strict-integer
Open

aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:json-strict-integer

Conversation

@aysha-afrah26

Copy link
Copy Markdown
Contributor

nlohmann's get<T>() for an integral T converts a JSON float with a bare static_cast, so a value outside the range of T is undefined behavior (UBSan float-cast-overflow), a fraction is truncated and an out-of-range integer wraps. Several readers apply it to external JSON: JsonExporter::fromJson picks std::vector<int> from the first array element only and then casts every element (ImportBlackboardFromJSON with {"v":[1, 1e300]}), a json:[1e300] value on a std::vector<int> port in the XML, async_delay in loadSubstitutionRuleFromJSON, and the uid/position/mode fields of the Groot2 HOOK_INSERT, BREAKPOINT_UNLOCK and HOOK_REMOVE requests. On the Groot2 server a uid of node_uid + 65536 or node_uid + 0.5 silently installs the hook on that real node, and 1e300 is UB on the server thread. UBSan reports 1e+300 is outside the range of representable values of type 'int' (json.hpp:4983) for the first three, and on master the hook request with a wrapped uid is accepted. #1186 fixed the scalar branch of fromJson; this covers the array branch and every other get<integral> call on external input in the tree (the BT_JSON_CONVERTER fields belong to user types and are left alone). The new inline JsonToInteger<T> in json_export.h accepts only JSON integers representable as T and throws RuntimeError otherwise, so a bad value takes the existing error paths; the Groot2 server already turns the exception into an error reply.

Behavior change: an integer array with a real or out-of-range element, and a fractional or out-of-range async_delay or hook uid, now throw instead of being converted. Inline header additions only, no ABI change. The new tests JsonTest.IntegerValuesAreRangeChecked, Substitution.ParserRejectsNonIntegerAsyncDelay and Groot2PublisherIntegration.HookRequestRejectsNonIntegerUid fail on master and pass with the change. Full ctest is 549/549 under ASan+UBSan on macOS, and clang-format and codespell are clean (clang-tidy-21 was not available locally).

json.get<T>() for an integral T converts a floating point number with a
plain static_cast, which is undefined behavior outside the range of T,
truncates fractions, and wraps out-of-range integers. Read those values
through a new JsonToInteger<T> helper that throws RuntimeError instead:
the elements of an integer array in JsonExporter::fromJson, the "json:"
form of convertFromString<std::vector<int>>, async_delay in
loadSubstitutionRuleFromJSON, and the uid/position/mode fields of the
Groot2 hook requests.

Behavior change: an integer array with a real or out-of-range element,
and a fractional or out-of-range async_delay or hook uid, now throw
instead of being silently converted.

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