Skip to content

Convert Packer's buf_size once - #726

Merged
methane merged 1 commit into
msgpack:mainfrom
hdimer:fix-buf-size-converted-twice
Aug 14, 2026
Merged

methane merged 1 commit into
msgpack:mainfrom
hdimer:fix-buf-size-converted-twice

Conversation

@hdimer

@hdimer hdimer commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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
@methane
methane merged commit 9f9bdae into msgpack:main Aug 14, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants