Repository navigation
fallback: leave the object unread when a header type does not match - #748
Merged
Merged
Conversation
read_array_header() and read_map_header() in the pure Python Unpacker consumed the object (header and, for str/bin/ext, its payload) before raising ValueError, so the next call started past it or in the middle of a container. The C extension leaves the object in place. Roll back to the start of the object, as the C extension does.
Member
|
@codex review please. |
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.
In the pure Python
Unpacker,read_array_header()andread_map_header()consume the next object before raisingValueErrorwhen it is not an array or map. The C extension leaves it in place, so code that catches the error and then callsunpack()behaves differently depending on which implementation is loaded:For a str, bin or ext the payload is skipped as well. For a map passed to
read_array_header(), only the map header is consumed, so the nextunpack()returns the first key and the stream is out of step from then on.tell()still reports the old offset, so it also disagrees with what was consumed.The fix rolls
_buff_iback to the buffer checkpoint (the start of the object) before raising, which is the same rollback_reserve()already does forOutOfDatain feed mode. The header readers are only called at the top level, so the checkpoint is always the start of the object.Tests:
test_incorrect_type_does_not_consume_objecttotest/test_read_size.py, which covers feed and file-like input. It fails withMSGPACK_PUREPYTHON=1before the change and passes after. It passes with the C extension both before and after.test/withMSGPACK_PUREPYTHON=1: 141 passed, 4 skipped. With the C extension: 144 passed, 1 skipped (Linux, CPython 3.12).unpack/skip/read_array_header/read_map_headercalls over feed and file input with severalread_sizevalues. 15,980 sequences differed before the change and 0 after.