Reject a mode change while an object is incomplete - #736
Draft
AbhinavMir wants to merge 1 commit into
Draft
Conversation
Unpacker.skip() does not build the objects on the parser stack. So stack[].obj stays uninitialized after an incomplete skip(). A later Unpacker.unpack() writes an array item through that pointer and the process crashes. The reverse order drops the reference that the construct pass took. Record the mode that started the object in unpack_context. unpack_construct() and unpack_skip() now raise ValueError on a change. The parser state stays intact, so the original mode still finishes the object.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The guard preserves parser state and the tests cover both unsafe mode-switch directions.
Pull request overview
Prevents unsafe switching between Unpacker.unpack() and skip() while parsing an incomplete container.
Changes:
- Tracks the active parser construction mode.
- Rejects incompatible mode switches with
ValueError. - Adds regression coverage for both switch directions and state recovery.
File summaries
| File | Description |
|---|---|
msgpack/unpack_template.h |
Adds mode tracking and validation. |
test/test_sequnpack.py |
Tests rejected mode switches and subsequent completion. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Unpackerkeeps its parser state between calls, so an incomplete object stays on the stack.unpack_execute()runs in two modes. Construct mode builds the objects. Skip mode does not.construct_cb()short circuits in skip mode, sostart_container()never setsstack[top].obj. That pointer stays uninitialized after an incompleteskip(). A laterunpack()reaches the_array_itemcall and writes an item through it. The process crashes.The reverse order is also wrong. An incomplete
unpack()builds a container and holds a reference to it. A laterskip()overwritesstack[0].objat_finishand drops that reference.unpack_contextrecorded nothing about the mode, and neitherunpack_construct()norunpack_skip()looked atctx->top. This change adds aconstructfield tounpack_context. Both entry points now compare it against the requested mode. They raiseValueErrorwhen an object is still open and the mode differs.The check runs before
unpack_execute(), so the parser state stays intact. The caller can still finish the object with the original method. The tests cover that.I used
ValueErrorto matchunpack_container_header.h, which raisesValueErrorfor input the parser rejects.RuntimeErrorin these files marks internal state corruption, so it did not fit.The new test is
test/test_sequnpack.py::test_mode_switch_while_incomplete. It covers both orders. The pure PythonUnpackerrestarts from a checkpoint and keeps no partial state, so it does not have this failure and does not need the guard. The test carries the sameskipifmarker that the other C extension tests use.