Refuse to resume a partial array/map with the other of skip()/unpack()

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:

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,87 @@ 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()
+    # 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()
+
+
+@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")
+    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]
+
+
+@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)