Reject a mode change while an object is incomplete

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.

Author: AbhinavMir

msgpack/unpack_template.h

@@ -35,6 +35,8 @@ struct unpack_context {
     unsigned int cs;
     unsigned int trail;
     unsigned int top;
+    /* mode that started the object on the stack. See unpack_check_mode(). */
+    bool construct;
     unpack_stack stack[MSGPACK_EMBED_STACK_SIZE];
 };
 
@@ -44,6 +46,7 @@ static inline void unpack_init(unpack_context* ctx)
     ctx->cs = CS_HEADER;
     ctx->trail = 0;
     ctx->top = 0;
+    ctx->construct = true;
     ctx->stack[0].obj = NULL;
 }
 
@@ -386,14 +389,37 @@ static inline int unpack_execute(bool construct, unpack_context* ctx, const char
 #undef again_fixed_trail_if_zero
 #undef start_container
 
+/*
+ * skip mode does not build the objects on the stack, so stack[].obj stays
+ * uninitialized. A later construct pass writes an item through that pointer.
+ * The reverse order drops the reference that the construct pass took.
+ * Reject the mode change while an object is still open.
+ */
+static inline int unpack_check_mode(unpack_context *ctx, bool construct)
+{
+    if (ctx->top != 0 && ctx->construct != construct) {
+        PyErr_SetString(PyExc_ValueError,
+                        "cannot switch between unpack and skip while an object is incomplete");
+        return -1;
+    }
+    ctx->construct = construct;
+    return 0;
+}
+
 static int unpack_construct(unpack_context *ctx, const char *data, Py_ssize_t len, Py_ssize_t *off) {
+    if (unpack_check_mode(ctx, true) < 0) {
+        return -1;
+    }
     int ret = unpack_execute(1, ctx, data, len, off);
     if (ret == -1) {
         unpack_clear(ctx);
     }
     return ret;
 }
 static int unpack_skip(unpack_context *ctx, const char *data, Py_ssize_t len, Py_ssize_t *off) {
+    if (unpack_check_mode(ctx, false) < 0) {
+        return -1;
+    }
     int ret = unpack_execute(0, ctx, data, len, off);
     if (ret == -1) {
         unpack_clear(ctx);

test/test_sequnpack.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python
 import io
 
-from pytest import raises
+from pytest import mark, raises
 
 from msgpack import BufferFull, Unpacker, pack, packb
 from msgpack.exceptions import OutOfData
@@ -146,3 +146,30 @@ def test_unpack_tell():
         m2 = next(unpacker)
         assert m == m2
         assert o == unpacker.tell()
+
+
+@mark.skipif(
+    Unpacker.__module__ == "msgpack.fallback",
+    reason="only the C extension keeps parser state between calls",
+)
+def test_mode_switch_while_incomplete():
+    # skip() does not build the objects that unpack() needs, so the parser
+    # state of one mode is not valid for the other. The unpacker must reject
+    # the change instead of a crash. The original mode still finishes.
+    unpacker = Unpacker()
+    unpacker.feed(b"\x91")
+    with raises(OutOfData):
+        unpacker.skip()
+    unpacker.feed(b"\x00")
+    with raises(ValueError):
+        unpacker.unpack()
+    assert unpacker.skip() is None
+
+    unpacker = Unpacker()
+    unpacker.feed(b"\x91")
+    with raises(OutOfData):
+        unpacker.unpack()
+    unpacker.feed(b"\x00")
+    with raises(ValueError):
+        unpacker.skip()
+    assert unpacker.unpack() == [0]