From aaf38ecad5cb5279b8c546af7217f7d177ed5867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Afonso=20Janu=C3=A1rio?= Date: Sun, 30 Aug 2026 12:33:41 +0100 Subject: [PATCH 1/2] Refuse to resume a partial array/map with the other of skip()/unpack() skip() and unpack() share one Unpacker._unpack() state machine and the same context, but a skip() call never populates a container frame's object slot, since it has no Python object to build there. If skip() leaves an array or map open because the buffer ran out mid-container, and the next call resumes it through unpack() instead, unpack() treats that slot as if it already held a real list or dict and crashes trying to append into it. Track which of the two last made progress on an in-progress container and reject switching to the other one with a clear ValueError instead of segfaulting. Resuming with the same method that started the parse still works exactly as before. Fixes #734 --- msgpack/_unpacker.pyx | 16 ++++++++++++++++ test/test_sequnpack.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/msgpack/_unpacker.pyx b/msgpack/_unpacker.pyx index 7e406622..f444472d 100644 --- a/msgpack/_unpacker.pyx +++ b/msgpack/_unpacker.pyx @@ -44,6 +44,7 @@ cdef extern from "unpack.h": msgpack_user user PyObject* obj Py_ssize_t count + unsigned int top ctypedef int (*execute_fn)(unpack_context* ctx, const char* data, Py_ssize_t len, Py_ssize_t* off) except -1 @@ -320,6 +321,10 @@ cdef class Unpacker: cdef Py_ssize_t max_buffer_size cdef uint64_t stream_offset cdef bint _unpacking + # Which of unpack_construct/unpack_skip left an array or map open on + # the last call, so a later call can refuse to resume it with the + # other one instead of touching stack entries it never populated. + cdef execute_fn _resume_execute def __dealloc__(self): unpack_clear(&self.ctx) @@ -473,6 +478,14 @@ cdef class Unpacker: cdef object obj cdef Py_ssize_t prev_head + if (self.ctx.top != 0 and self._resume_execute != NULL + and self._resume_execute != execute): + raise ValueError( + "unpack() and skip() cannot be mixed while an array or " + "map is still incomplete; finish it with the same method " + "that started it" + ) + self._unpacking = True try: while 1: @@ -486,8 +499,10 @@ cdef class Unpacker: if ret == 1: obj = unpack_data(&self.ctx) unpack_init(&self.ctx) + self._resume_execute = NULL return obj if ret == 0: + self._resume_execute = execute if self.file_like is not None: self.read_from_file() continue @@ -497,6 +512,7 @@ cdef class Unpacker: raise OutOfData("No more data to unpack.") unpack_clear(&self.ctx) + self._resume_execute = NULL if ret == -2: raise FormatError elif ret == -3: diff --git a/test/test_sequnpack.py b/test/test_sequnpack.py index 0f895d7d..ae1ad440 100644 --- a/test/test_sequnpack.py +++ b/test/test_sequnpack.py @@ -63,6 +63,48 @@ def test_foobar_skip(): unpacker.unpack() +def test_skip_then_unpack_across_incomplete_container(): + # skip() opens the array's stack frame without ever populating its + # object slot (it has nothing to build), so resuming with unpack() + # used to reuse that slot as if it held a real list and crash. See + # GH #734. + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.skip() + unpacker.feed(b"\x00") + with raises(ValueError): + unpacker.unpack() + + +def test_unpack_then_skip_across_incomplete_container(): + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.unpack() + unpacker.feed(b"\x00") + with raises(ValueError): + unpacker.skip() + + +def test_skip_then_skip_across_incomplete_container_still_works(): + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.skip() + unpacker.feed(b"\x00") + assert unpacker.skip() is None + + +def test_unpack_then_unpack_across_incomplete_container_still_works(): + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.unpack() + unpacker.feed(b"\x00") + assert unpacker.unpack() == [0] + + def test_maxbuffersize(): with raises(ValueError): Unpacker(read_size=5, max_buffer_size=3) From 10a8149f39eea66e6f94e1588c3d29e31f924af7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Afonso=20Janu=C3=A1rio?= Date: Sun, 30 Aug 2026 12:39:00 +0100 Subject: [PATCH 2/2] Skip the mode-mix regression tests on the pure Python fallback The fallback rolls its buffer position back to the last checkpoint on every OutOfData, so an incomplete read never leaves behind a stack frame the way the C extension does. Resuming with the other method just reparses the header from scratch and works fine there, so the ValueError the C extension raises for that mix isn't something the fallback needs to reproduce. Marked the two mix tests C-extension-only with the project's existing Packer.__module__ skipif convention, and added the fallback's own pair of tests confirming the mix still succeeds instead of silently going untested on that backend. --- test/test_sequnpack.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/test_sequnpack.py b/test/test_sequnpack.py index ae1ad440..e0613939 100644 --- a/test/test_sequnpack.py +++ b/test/test_sequnpack.py @@ -1,6 +1,7 @@ #!/usr/bin/env python import io +import pytest from pytest import raises from msgpack import BufferFull, Unpacker, pack, packb @@ -63,6 +64,10 @@ def test_foobar_skip(): unpacker.unpack() +@pytest.mark.skipif( + Unpacker.__module__ == "msgpack.fallback", + reason="only the C extension keeps a stack frame across an incomplete read", +) def test_skip_then_unpack_across_incomplete_container(): # skip() opens the array's stack frame without ever populating its # object slot (it has nothing to build), so resuming with unpack() @@ -77,6 +82,10 @@ def test_skip_then_unpack_across_incomplete_container(): unpacker.unpack() +@pytest.mark.skipif( + Unpacker.__module__ == "msgpack.fallback", + reason="only the C extension keeps a stack frame across an incomplete read", +) def test_unpack_then_skip_across_incomplete_container(): unpacker = Unpacker() unpacker.feed(b"\x91") @@ -105,6 +114,37 @@ def test_unpack_then_unpack_across_incomplete_container_still_works(): assert unpacker.unpack() == [0] +@pytest.mark.skipif( + Unpacker.__module__ != "msgpack.fallback", + reason="the C extension is the one that needs to reject this mix, see the tests above", +) +def test_fallback_skip_then_unpack_across_incomplete_container_still_works(): + # The fallback never keeps a stack frame across an OutOfData; an + # incomplete read rolls the buffer position back to where the call + # started, so the next call just reparses the array header from + # scratch regardless of which method it uses. No corruption risk here, + # so unlike the C extension it doesn't need to reject the mix. + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.skip() + unpacker.feed(b"\x00") + assert unpacker.unpack() == [0] + + +@pytest.mark.skipif( + Unpacker.__module__ != "msgpack.fallback", + reason="the C extension is the one that needs to reject this mix, see the tests above", +) +def test_fallback_unpack_then_skip_across_incomplete_container_still_works(): + unpacker = Unpacker() + unpacker.feed(b"\x91") + with raises(OutOfData): + unpacker.unpack() + unpacker.feed(b"\x00") + assert unpacker.skip() is None + + def test_maxbuffersize(): with raises(ValueError): Unpacker(read_size=5, max_buffer_size=3)