Refuse to resize the packer buffer while a memoryview is exported - #739
Open
afonsojanu wants to merge 1 commit into
Open
Refuse to resize the packer buffer while a memoryview is exported#739afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
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 msgpackGH-733.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #733.
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 growing the buffer mid-pack can move it out from under an export that Python still considers valid. Under ASan this shows up as a straightforward heap-use-after-free the moment anything reads through the export afterward, matching what's reported in the issue.
The exports counter used to live only on the Cython Packer object, so the plain C write path in pack.h had no way to see it. I moved it into the msgpack_packer struct itself and made msgpack_pack_write raise BufferError instead of reallocating whenever exports is nonzero and growth is needed - the same error _check_exports() already raises for every other buffer-mutating method, just reachable from the one path that runs mid-pack rather than at the start of a call.
Added a regression test (buf_size set small enough that the default() return value forces a realloc) confirming it raises BufferError with the fix and silently proceeds without it. Full suite passes locally (139 passed, 1 pre-existing skip).
I wasn't able to get an ASan-instrumented build running in the time I had, so I haven't reproduced the crash itself the way the issue's reporter did - the fix is based on reading the allocation/export lifecycle directly rather than on catching it under a sanitizer, and the raised BufferError is verified to actually happen with git stash on both sides of the change.