Repository navigation
gh-157335: Fix out-of-bounds write in mmap.mmap.__setitem__ - #157438
vstinner merged 16 commits into
Conversation
Sorry, something went wrong.
…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. |
Sorry, something went wrong.
| /* 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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
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.
Sorry, something went wrong.
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
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, something went wrong.
|
Sorry, @Joekrry and @vstinner, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker 09bf4c525871db3ad02c8a1359da5cf0668f27b0 3.13 |
Sorry, something went wrong.
|
GH-158026 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
GH-158027 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-158028 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…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>
Assigning to a single index in mmap.mmap.__setitem__ validates the index against the object's size, then converts the assigned value via PyNumber_AsSsize_t(). That conversion can invoke arbitrary Python code through __index__(), and if that code calls mmap.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.