Skip to content

Commit 8ed8226

Browse files
committed
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.
1 parent 7a63920 commit 8ed8226

3 files changed

Lines changed: 44 additions & 7 deletions

File tree

msgpack/_packer.pyx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ cdef extern from "pack.h":
2222
size_t length
2323
size_t buf_size
2424
bint use_bin_type
25+
size_t exports
2526

2627
int msgpack_pack_nil(msgpack_packer* pk) except -1
2728
int msgpack_pack_true(msgpack_packer* pk) except -1
@@ -105,7 +106,6 @@ cdef class Packer:
105106
cdef object _default
106107
cdef object _berrors
107108
cdef const char *unicode_errors
108-
cdef size_t exports # number of exported buffers
109109
cdef bint strict_types
110110
cdef bint use_float
111111
cdef bint autoreset
@@ -117,15 +117,15 @@ cdef class Packer:
117117
raise MemoryError("Unable to allocate internal buffer.")
118118
self.pk.buf_size = buf_size
119119
self.pk.length = 0
120-
self.exports = 0
120+
self.pk.exports = 0
121121

122122
def __dealloc__(self):
123123
PyMem_Free(self.pk.buf)
124124
self.pk.buf = NULL
125-
assert self.exports == 0
125+
assert self.pk.exports == 0
126126

127127
cdef _check_exports(self):
128-
if self.exports > 0:
128+
if self.pk.exports > 0:
129129
raise BufferError("Existing exports of data: Packer cannot be changed")
130130

131131
@cython.critical_section
@@ -364,8 +364,8 @@ cdef class Packer:
364364
@cython.critical_section
365365
def __getbuffer__(self, Py_buffer *buffer, int flags):
366366
PyBuffer_FillInfo(buffer, self, self.pk.buf, self.pk.length, 1, flags)
367-
self.exports += 1
367+
self.pk.exports += 1
368368

369369
@cython.critical_section
370370
def __releasebuffer__(self, Py_buffer *buffer):
371-
self.exports -= 1
371+
self.pk.exports -= 1

msgpack/pack.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ typedef struct msgpack_packer {
3232
size_t length;
3333
size_t buf_size;
3434
bool use_bin_type;
35+
size_t exports;
3536
} msgpack_packer;
3637

3738
typedef struct Packer Packer;
@@ -43,6 +44,16 @@ static inline int msgpack_pack_write(msgpack_packer* pk, const char *data, size_
4344
size_t len = pk->length;
4445

4546
if (len + l > bs) {
47+
if (pk->exports > 0) {
48+
/* A `default` callback (or anything else running mid-pack) holds a
49+
* live memoryview onto this buffer via getbuffer(). Growing the
50+
* buffer here would realloc it out from under that memoryview,
51+
* since PyMem_Realloc is free to move the allocation, leaving the
52+
* export pointing at freed memory. */
53+
PyErr_SetString(PyExc_BufferError,
54+
"Existing exports of data: cannot resize packer's internal buffer");
55+
return -1;
56+
}
4657
bs = (len + l) * 2;
4758
buf = (char*)PyMem_Realloc(buf, bs);
4859
if (!buf) {

test/test_pack.py

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,8 +203,34 @@ def test_get_buffer():
203203

204204
@pytest.mark.skipif(
205205
Packer.__module__ == "msgpack.fallback",
206-
reason="buf_size only allocates in the C extension",
206+
reason="the fallback packer's getbuffer() returns a BytesIO view, not a view onto a reallocatable C buffer",
207207
)
208+
def test_pack_growth_rejected_while_buffer_exported():
209+
# A default() callback that calls getbuffer() mid-pack holds a live
210+
# memoryview onto the packer's internal buffer. If the packer then needs
211+
# to grow that buffer to fit more data, realloc() is free to move the
212+
# allocation, leaving the export pointing at freed memory (a
213+
# heap-use-after-free once anything reads through it).
214+
exported = []
215+
216+
class Unsupported:
217+
pass
218+
219+
def default(obj):
220+
exported.append(packer.getbuffer())
221+
# Comfortably bigger than buf_size, so packing this forces a realloc.
222+
return b"x" * 4096
223+
224+
packer = Packer(default=default, autoreset=False, buf_size=64)
225+
226+
with pytest.raises(BufferError):
227+
packer.pack(Unsupported())
228+
229+
# The export is still alive and still backed by real memory; releasing it
230+
# shouldn't touch anything that was already freed.
231+
del exported[:]
232+
233+
208234
def test_buf_size_is_converted_once():
209235
# Asking twice let the allocation and the recorded capacity disagree,
210236
# so the packer overflowed a buffer smaller than the size it recorded.

0 commit comments

Comments
 (0)