From 8ed8226f18055e28e93455fa84517f70e9d32b5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Afonso=20Janu=C3=A1rio?= Date: Fri, 4 Sep 2026 13:52:32 +0100 Subject: [PATCH] Refuse to resize the packer buffer while a memoryview is exported Packer.pack() checks for existing buffer exports before it starts, but nothing stopped a default() callback from calling getbuffer() partway through the same call and then having the packer grow its buffer to fit the rest of the object. msgpack_pack_write() reallocates through PyMem_Realloc without checking whether anything holds a live view onto the old allocation, so a growth mid-pack can move the buffer out from under an export that's still considered valid from Python's side. An ASan build turns this into a textbook heap-use-after-free the moment anything reads through the export afterward. Moved the exports counter into the msgpack_packer C struct itself (previously it only lived on the Cython Packer object, invisible to the plain C write path) and made msgpack_pack_write raise BufferError instead of reallocating whenever exports is nonzero and the buffer needs to grow. This is the same error _check_exports() already raises for every other buffer-mutating method, just reachable from the one code path that runs in the middle of a pack() call rather than at its start. Fixes GH-733. --- msgpack/_packer.pyx | 12 ++++++------ msgpack/pack.h | 11 +++++++++++ test/test_pack.py | 28 +++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/msgpack/_packer.pyx b/msgpack/_packer.pyx index 52f4f1f2..598918e9 100644 --- a/msgpack/_packer.pyx +++ b/msgpack/_packer.pyx @@ -22,6 +22,7 @@ cdef extern from "pack.h": size_t length size_t buf_size bint use_bin_type + size_t exports int msgpack_pack_nil(msgpack_packer* pk) except -1 int msgpack_pack_true(msgpack_packer* pk) except -1 @@ -105,7 +106,6 @@ cdef class Packer: cdef object _default cdef object _berrors cdef const char *unicode_errors - cdef size_t exports # number of exported buffers cdef bint strict_types cdef bint use_float cdef bint autoreset @@ -117,15 +117,15 @@ cdef class Packer: raise MemoryError("Unable to allocate internal buffer.") self.pk.buf_size = buf_size self.pk.length = 0 - self.exports = 0 + self.pk.exports = 0 def __dealloc__(self): PyMem_Free(self.pk.buf) self.pk.buf = NULL - assert self.exports == 0 + assert self.pk.exports == 0 cdef _check_exports(self): - if self.exports > 0: + if self.pk.exports > 0: raise BufferError("Existing exports of data: Packer cannot be changed") @cython.critical_section @@ -364,8 +364,8 @@ cdef class Packer: @cython.critical_section def __getbuffer__(self, Py_buffer *buffer, int flags): PyBuffer_FillInfo(buffer, self, self.pk.buf, self.pk.length, 1, flags) - self.exports += 1 + self.pk.exports += 1 @cython.critical_section def __releasebuffer__(self, Py_buffer *buffer): - self.exports -= 1 + self.pk.exports -= 1 diff --git a/msgpack/pack.h b/msgpack/pack.h index edf3a3fe..abc7e2f8 100644 --- a/msgpack/pack.h +++ b/msgpack/pack.h @@ -32,6 +32,7 @@ typedef struct msgpack_packer { size_t length; size_t buf_size; bool use_bin_type; + size_t exports; } msgpack_packer; typedef struct Packer Packer; @@ -43,6 +44,16 @@ static inline int msgpack_pack_write(msgpack_packer* pk, const char *data, size_ size_t len = pk->length; if (len + l > bs) { + if (pk->exports > 0) { + /* A `default` callback (or anything else running mid-pack) holds a + * live memoryview onto this buffer via getbuffer(). Growing the + * buffer here would realloc it out from under that memoryview, + * since PyMem_Realloc is free to move the allocation, leaving the + * export pointing at freed memory. */ + PyErr_SetString(PyExc_BufferError, + "Existing exports of data: cannot resize packer's internal buffer"); + return -1; + } bs = (len + l) * 2; buf = (char*)PyMem_Realloc(buf, bs); if (!buf) { diff --git a/test/test_pack.py b/test/test_pack.py index 9ca6e182..b8ccb5d0 100644 --- a/test/test_pack.py +++ b/test/test_pack.py @@ -203,8 +203,34 @@ def test_get_buffer(): @pytest.mark.skipif( Packer.__module__ == "msgpack.fallback", - reason="buf_size only allocates in the C extension", + reason="the fallback packer's getbuffer() returns a BytesIO view, not a view onto a reallocatable C buffer", ) +def test_pack_growth_rejected_while_buffer_exported(): + # A default() callback that calls getbuffer() mid-pack holds a live + # memoryview onto the packer's internal buffer. If the packer then needs + # to grow that buffer to fit more data, realloc() is free to move the + # allocation, leaving the export pointing at freed memory (a + # heap-use-after-free once anything reads through it). + exported = [] + + class Unsupported: + pass + + def default(obj): + exported.append(packer.getbuffer()) + # Comfortably bigger than buf_size, so packing this forces a realloc. + return b"x" * 4096 + + packer = Packer(default=default, autoreset=False, buf_size=64) + + with pytest.raises(BufferError): + packer.pack(Unsupported()) + + # The export is still alive and still backed by real memory; releasing it + # shouldn't touch anything that was already freed. + del exported[:] + + def test_buf_size_is_converted_once(): # Asking twice let the allocation and the recorded capacity disagree, # so the packer overflowed a buffer smaller than the size it recorded.