gh-157335: Fix out-of-bounds write in mmap.mmap.__setitem__ - #157438
Conversation
…Assigning to a single index causes mmap.mmap.__setitem__ to validate the index against the object's size, which then converts the assigned value via PyNumber_AsSsize_t(). This invokes arbitrary python code via __index__(). mmap.resize(), which shrinks mapping could point past the end fo the new buffer causing an out of bounds error write.
|
All commit authors signed the Contributor License Agreement. |
| /* value's __index__ may have resized the mmap, invalidating | ||
| * the earlier bounds check on i. */ | ||
| if (i >= self->size) { | ||
| PyErr_SetString(PyExc_IndexError, | ||
| "mmap index out of range"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I think we can have the same issue in the slice-branch when doing PyObject_GetBuffer(). So instead, we could make the checks inside the safe_byte_copy and safe_memcpy functions. Though I don't know if it's an overkill. Can you verify that the slice pah is also not affected by adding tests.
There was a problem hiding this comment.
Yeah, I fixed the slice path so PyObject_GetBuffer is called first, then computes bounds with PySlice_AdjustIndices against the size after that call. Added a test for this as well.
I didn't move the checks because the check still has to occur at the call.
Hopefully my understanding of this is correct
Fix the same reentrancy issue in the slice-assignment path: acquiring the value's buffer (e.g. via a __buffer__ method) can also run arbitrary code that resizes the mmap, invalidating the previously computed slice bounds. Re-validate the bounds after acquiring the buffer, before copying into the mapping. Clarify why the single-item path still needs a bounds check both before and after converting the value: the earlier check preserves existing error precedence (IndexError before TypeError, per test_basic), while the later one is a narrow revalidation of just the upper bound.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Thanks, the PR looks better with checks moved after functions which can call arbitrary Python code. New review: you can now remove redundant CHECK_VALID() check.
Co-authored-by: Victor Stinner <vstinner@python.org>
…faMah.rst Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
added
needs backport to 3.13
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
@serhiy-storchaka: Do you want to double check this mmap fix?
@Joekrry: I'm not sure why you're merging the main branch into your branch so often, it's not need and it makes the review harder to follow :-(
|
Sorry, @Joekrry and @vstinner, I could not cleanly backport this to |
|
GH-158026 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
GH-158027 is a backport of this pull request to the 3.14 branch. |
|
GH-158028 is a backport of this pull request to the 3.13 branch. |
|
Merged. Thanks for the fix @Joekrry! |
…157438) (#158028) gh-157335: Fix out-of-bounds write in mmap.mmap.__setitem__ (#157438) Fix out-of-bounds write in mmap.mmap.__setitem__() that could occur when converting the index or the assigned value (via __index__() for a single item, or via the buffer protocol for a slice) resized or closed the mmap object during the assignment. (cherry picked from commit 09bf4c5) Co-authored-by: Joseph Kerry <joerkerry@gmail.com>
…H-157438) (#158027) gh-157335: Fix out-of-bounds write in mmap.mmap.__setitem__ (GH-157438) Fix out-of-bounds write in mmap.mmap.__setitem__() that could occur when converting the index or the assigned value (via __index__() for a single item, or via the buffer protocol for a slice) resized or closed the mmap object during the assignment. (cherry picked from commit 09bf4c5) Co-authored-by: Joseph Kerry <joerkerry@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>
3 participants
Assigning to a single index in
mmap.mmap.__setitem__validates the index against the object's size, then converts the assigned value viaPyNumber_AsSsize_t(). That conversion can invoke arbitrary Python code through__index__(), and if that code callsmmap.resize()to shrink the mapping, the previously validated index can point past the end of the new, smaller buffer — causing an out-of-bounds write.This re-validates the index against the mmap's current size after the value conversion, right alongside the existing
CHECK_VALID()check that already accounts for reentrancy at that point.Fixes gh-157335.