Skip to content

Convert Packer's buf_size once - #726

Open
hdimer wants to merge 1 commit into
msgpack:mainfrom
hdimer:fix-buf-size-converted-twice
Open

Convert Packer's buf_size once#726
hdimer wants to merge 1 commit into
msgpack:mainfrom
hdimer:fix-buf-size-converted-twice

Conversation

@hdimer

@hdimer hdimer commented Aug 12, 2026

Copy link
Copy Markdown

Fixes #723.

Packer.__cinit__ takes buf_size as an untyped object, and Cython converts it to size_t separately at each of the two lines that use it:

def __cinit__(self, buf_size=256*1024, **_kwargs):
    self.pk.buf = <char*> PyMem_Malloc(buf_size)   # converts once
    if self.pk.buf == NULL:
        raise MemoryError("Unable to allocate internal buffer.")
    self.pk.buf_size = buf_size                    # converts again

An object that answers differently each time therefore sizes the allocation from one answer and the recorded capacity from the other. Everything afterwards trusts the recorded capacity:

if (len + l > bs) {   /* pack.h:45 - bs is pk.buf_size */
    ...grow...
}
memcpy(buf + len, data, l);

so a payload that fits the recorded capacity but not the real block is copied straight past the allocation. On the reporter's example the packer holds 600 bytes, records 1 MiB, and pack(b"A" * 100000) writes 100000 bytes into the 600-byte block.

The fix

Type the parameter, so the conversion happens once during argument unpacking:

def __cinit__(self, size_t buf_size=256*1024, **_kwargs):

This is how Unpacker already takes read_size and max_buffer_size (_unpacker.pyx:330), and it leaves no local for a later reader to fold back. It also removes a smaller wart: previously a second conversion that raised would leave __cinit__ failing with pk.buf already allocated.

Same converter (__Pyx_PyInt_As_size_t), so nothing user-visible moves. I checked the constructor's edge cases against a pre-fix build and they are identical, message for message: -1 gives OverflowError: can't convert negative value to size_t, 1 << 62 gives MemoryError, "x" and None give TypeError: an integer is required, and 0, 1.5, True and the default are accepted as before.

Tests

test_buf_size_is_converted_once asserts the object is queried exactly once. The overflow itself cannot be asserted portably (it takes the interpreter down rather than failing a test), so the test pins the invariant that prevents it. It fails on main with assert 2 == 1 and passes with the change.

The skipif follows the existing convention in test_unpack.py, since fallback.py accepts buf_size and ignores it. The helper's __int__ is the load-bearing slot on Cython 3.2.5, with __index__ aliased to it so the test keeps counting if a future Cython routes through PyNumber_Index instead.

Verified on CPython 3.14: full suite green in both the C-extension and MSGPACK_PUREPYTHON=1 configurations, and ruff check / ruff format clean. The reporter's reproducer no longer crashes.

Disclosure: I used an AI assistant while investigating and writing this patch. I reproduced the overflow myself, confirmed the pre-fix crash and the edge-case parity against a locally built extension, and I stand behind the change.

buf_size was untyped, so Cython converted it to size_t separately for
the PyMem_Malloc call and for pk.buf_size. An object whose __int__
answers differently each call made the packer allocate one size and
record another, and pack.h then grew the buffer against the recorded
capacity, so a large enough payload was memcpy'd past the allocation.

Typing the parameter converts it once during argument unpacking, the
same way Unpacker takes read_size and max_buffer_size.

Fixes msgpack#723
@hdimer
hdimer marked this pull request as ready for review August 12, 2026 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heap buffer overflow in Packer(buf_size=…) with an unstable __index__

1 participant