Read CBOR containers and tags without recursing per nesting level - #5506
Read CBOR containers and tags without recursing per nesting level#5506nlohmann wants to merge 2 commits into
Conversation
| case 0x97: | ||
| return get_cbor_array( | ||
| conditional_static_cast<std::size_t>(static_cast<unsigned int>(current) & 0x1Fu), tag_handler); | ||
| return enter_container(/*is_object*/false, |
68e4094 to
ed0f44d
Compare
ed0f44d to
baeb85a
Compare
1df654d to
b091401
Compare
b091401 to
6ea776c
Compare
6ea776c to
48bbe7c
Compare
get_cbor_array() and get_cbor_object() read their elements by calling back into the value reader, which called them again for a nested container, and a tag was handled by reading the tagged value the same way. All three cost native stack, and all three cost a single byte to encode: 0x9F opens an indefinite-length array, 0x81 a one-element array, and 0xC2 is a tag. Half a million of any of them crashes the process before the input runs out (#5104). Apply the shape the MessagePack reader already uses: the open containers live on the heap stack, parse_cbor_value() reads a single value and only opens a container rather than reading it to its end, and parse_cbor_internal() loops, resuming the innermost container after each element. Two things are specific to CBOR. An indefinite-length container ends at a break marker rather than at a count, and testing for that marker consumes a byte which is the first byte of the next element when it is not one; the frame's count is npos for those, and the driver tracks whether the next value starts at a fresh byte. And a tag is not a value of its own: instead of reading the tagged value by recursing, the value reader reports that a tag was read and the driver reads on, so a chain of tags costs no stack at all. The switch that decodes a value is unchanged apart from the twelve container cases and the two tag sites. Verified against the previous commit over definite and indefinite arrays and maps, all four counted forms, empty containers, nesting of the forms inside each other, truncated inputs, and all three tag handlers: identical values, error codes, messages and byte offsets. 500,000 levels of each of the three vectors now report parse_error.110 instead of crashing, and a well-formed 200,000-level value is read to completion. On performance: the driver does per element what a counted loop used to do per container, and CBOR pays for it more than MessagePack because the value reader also has to be told whether to fetch a byte. Parsing 60,000 small objects and one array of a million integers is 3 to 4 % slower than the recursive reader, measured over five alternating runs. Against develop the same two inputs are about 44 % faster, because the entry point no longer copies the value it parsed; the earlier commit in this series is what pays for that. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
48bbe7c to
4297d2a
Compare
| get(); | ||
| if (JSON_HEDLEY_UNLIKELY(!get_cbor_string(key) || !sax->key(key))) | ||
| const bool is_object = top.is_object; | ||
| container_stack.pop_back(); |
There was a problem hiding this comment.
This invalidates top. There should be a comment here about that, so someone doesn't change something later that allows top to be read again. Alternatively, top could be a copy, and then --top.remaining; above becomes --container_stack.back().remaining;.
There was a problem hiding this comment.
Fixed in d05ac29 - went with the copy approach you suggested, and updated the comment above to explain why (it is not about reallocation from a push anymore, it is about surviving the pop_back() here).
Written by Claude Code.
| if (!container_stack.empty()) | ||
| { | ||
| if (JSON_HEDLEY_UNLIKELY(!parse_cbor_internal(true, tag_handler))) | ||
| // the reference is not held across parse_cbor_value() below, |
There was a problem hiding this comment.
However, it is held across container_stack.pop_back();, comments below about mitigations or fixes.
There was a problem hiding this comment.
Fixed in d05ac29: top is now a copy (container_frame top = container_stack.back();), so it stays valid regardless of what happens to container_stack afterward, including the pop_back() right below. The one mutation (--top.remaining) now goes through container_stack.back() directly.
Written by Claude Code.
top aliased container_stack.back(), and was still read (top.is_object) right after container_stack.pop_back() destroyed the element it aliased. Nothing currently reorders those two lines, but the comment claiming the reference's lifetime was already fine only accounted for reallocation from a push, not this. A trivially-copyable container_frame makes top a copy instead, so reads of it stay valid regardless of what happens to the stack; the one place that mutates the live entry now does so through container_stack.back() directly rather than through top. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Closes the CBOR container and tag vectors of #5104. CBOR has the most open OSS-Fuzz stack-overflow reports of any format here (issues 487903138 and 371248057).
What
get_cbor_array()andget_cbor_object()read their elements by calling back into the value reader, which called them again for a nested container, and a tag was handled by reading the tagged value the same way. All three cost native stack, and all three cost a single byte to encode:0x9F0x810xC2Half a million of any of them crashes the process before the input runs out.
How
The shape the MessagePack reader already uses (#5505): the open containers live on the heap stack,
parse_cbor_value()reads a single value and only opens a container, andparse_cbor_internal()loops.Two things are specific to CBOR:
nposfor those, and the driver tracks whether the next value starts at a fresh byte.The switch that decodes a value is unchanged apart from the twelve container cases and the two tag sites.
Verification
unit-cborpasses unchanged (1.64 M assertions), plus 20 new ones.parse_error.110instead of crashing; a well-formed 200,000-level value is read to completion.Performance
Reported in full because it is not free. The driver does per element what a counted loop used to do per container, and CBOR pays for it more than MessagePack because the value reader also has to be told whether to fetch a byte. Over five alternating runs, parsing 60,000 small objects and one array of a million integers:
about 3–4 % slower. Against
develop, however, the same two inputs are about 44 % faster (19.90 → 11.21 ms), because #5501 removes the copy the entry point used to make.API impact
No breaking changes. Same inputs accepted, same errors, same byte offsets, same SAX event sequence.
Checklist
make amalgamate.🤖 Generated with Claude Code