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

Author: afonsojanu

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

@@ -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)