Skip to content

gh-103987: fix crash in mmap module #103990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 39 commits into from
May 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
d5cf0dd
gh-103987: fix crash in mmap module
Agent-Hellboy Apr 29, 2023
af99baa
fix test
Agent-Hellboy Apr 29, 2023
6b7af10
fix test
Agent-Hellboy Apr 29, 2023
6ab6a70
fix test
Agent-Hellboy Apr 29, 2023
50cc20b
📜🤖 Added by blurb_it.
blurb-it[bot] Apr 29, 2023
4d26d53
fix doc
Agent-Hellboy Apr 29, 2023
d8a1365
fix test
Agent-Hellboy Apr 29, 2023
428338f
add new test
Agent-Hellboy Apr 30, 2023
0a8213f
fix test
Agent-Hellboy Apr 30, 2023
b460efe
add checks in mmap_ass_subscript
Agent-Hellboy Apr 30, 2023
55bd26e
fix test
Agent-Hellboy Apr 30, 2023
7adecc4
fix test
Agent-Hellboy Apr 30, 2023
caab85b
Merge branch 'main' into fix-issue-103987
sunmy2019 May 12, 2023
d8b9897
Update Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.s…
sunmy2019 May 12, 2023
6d50192
gh-103987: fix crash in mmap module
Agent-Hellboy Apr 29, 2023
3784382
fix test
Agent-Hellboy Apr 29, 2023
5e19249
fix test
Agent-Hellboy Apr 29, 2023
b0d1fcd
fix test
Agent-Hellboy Apr 29, 2023
1482ee7
📜🤖 Added by blurb_it.
blurb-it[bot] Apr 29, 2023
8c76209
fix doc
Agent-Hellboy Apr 29, 2023
2018297
fix test
Agent-Hellboy Apr 29, 2023
2cea232
add new test
Agent-Hellboy Apr 30, 2023
e0de742
fix test
Agent-Hellboy Apr 30, 2023
5389a41
add checks in mmap_ass_subscript
Agent-Hellboy Apr 30, 2023
b5e37d7
fix test
Agent-Hellboy Apr 30, 2023
ed1715e
fix test
Agent-Hellboy Apr 30, 2023
49d1094
Update Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.s…
sunmy2019 May 12, 2023
61cdd6e
Add new test
Agent-Hellboy May 12, 2023
5c8d709
Merge branch 'fix-issue-103987' of https://github.com/Agent-Hellboy/c…
Agent-Hellboy May 12, 2023
341b90a
Update Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.s…
sunmy2019 May 12, 2023
6dece9f
Update Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.s…
sunmy2019 May 12, 2023
5cf8f90
fix test
Agent-Hellboy May 12, 2023
93163d9
Merge branch 'fix-issue-103988' of https://github.com/Agent-Hellboy/c…
Agent-Hellboy May 12, 2023
3b61c74
Merge branch 'main' into fix-issue-103987
sunmy2019 May 17, 2023
f2d2d00
Merge branch 'main' into fix-issue-103987
sunmy2019 May 19, 2023
843c2eb
add more checks, fix test cases
sunmy2019 May 19, 2023
168f5b1
Update Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.s…
sunmy2019 May 19, 2023
435ed41
remove trailing spaces
sunmy2019 May 19, 2023
320feac
Update Lib/test/test_mmap.py
sunmy2019 May 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 86 additions & 1 deletion Lib/test/test_mmap.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ def test_move(self):
m.move(0, 0, 1)
m.move(0, 0, 0)


def test_anonymous(self):
# anonymous mmap.mmap(-1, PAGE)
m = mmap.mmap(-1, PAGESIZE)
Expand Down Expand Up @@ -887,6 +886,92 @@ def test_resize_succeeds_with_error_for_second_named_mapping(self):
self.assertEqual(m1[:data_length], data)
self.assertEqual(m2[:data_length], data)

def test_mmap_closed_by_int_scenarios(self):
"""
gh-103987: Test that mmap objects raise ValueError
for closed mmap files
"""

class MmapClosedByIntContext:
def __init__(self, access) -> None:
self.access = access

def __enter__(self):
self.f = open(TESTFN, "w+b")
self.f.write(random.randbytes(100))
self.f.flush()

m = mmap.mmap(self.f.fileno(), 100, access=self.access)

class X:
def __index__(self):
m.close()
return 10

return (m, X)

def __exit__(self, exc_type, exc_value, traceback):
self.f.close()

read_access_modes = [
mmap.ACCESS_READ,
mmap.ACCESS_WRITE,
mmap.ACCESS_COPY,
mmap.ACCESS_DEFAULT,
]

write_access_modes = [
mmap.ACCESS_WRITE,
mmap.ACCESS_COPY,
mmap.ACCESS_DEFAULT,
]

for access in read_access_modes:
with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[X()]

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[X() : 20]

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[X() : 20 : 2]

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[20 : X() : -2]

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m.read(X())

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m.find(b"1", 1, X())

for access in write_access_modes:
with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[X() : 20] = b"1" * 10

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[X() : 20 : 2] = b"1" * 5

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m[20 : X() : -2] = b"1" * 5

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m.move(1, 2, X())

with MmapClosedByIntContext(access) as (m, X):
with self.assertRaisesRegex(ValueError, "mmap closed or invalid"):
m.write_byte(X())

class LargeMmapTests(unittest.TestCase):

def setUp(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
In :mod:`mmap`, fix several bugs that could lead to access to memory-mapped files after
they have been invalidated.
15 changes: 13 additions & 2 deletions Modules/mmapmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ mmap_read_method(mmap_object *self,

CHECK_VALID(NULL);
if (!PyArg_ParseTuple(args, "|O&:read", _Py_convert_optional_to_ssize_t, &num_bytes))
return(NULL);
return NULL;
CHECK_VALID(NULL);

/* silently 'adjust' out-of-range requests */
remaining = (self->pos < self->size) ? self->size - self->pos : 0;
Expand Down Expand Up @@ -325,6 +326,7 @@ mmap_gfind(mmap_object *self,
end = self->size;

Py_ssize_t res;
CHECK_VALID(NULL);
if (reverse) {
res = _PyBytes_ReverseFind(
self->data + start, end - start,
Expand Down Expand Up @@ -388,7 +390,7 @@ mmap_write_method(mmap_object *self,

CHECK_VALID(NULL);
if (!PyArg_ParseTuple(args, "y*:write", &data))
return(NULL);
return NULL;

if (!is_writable(self)) {
PyBuffer_Release(&data);
Expand All @@ -401,6 +403,7 @@ mmap_write_method(mmap_object *self,
return NULL;
}

CHECK_VALID(NULL);
memcpy(&self->data[self->pos], data.buf, data.len);
self->pos += data.len;
PyBuffer_Release(&data);
Expand All @@ -420,6 +423,7 @@ mmap_write_byte_method(mmap_object *self,
if (!is_writable(self))
return NULL;

CHECK_VALID(NULL);
if (self->pos < self->size) {
self->data[self->pos++] = value;
Py_RETURN_NONE;
Expand Down Expand Up @@ -724,6 +728,7 @@ mmap_move_method(mmap_object *self, PyObject *args)
if (self->size - dest < cnt || self->size - src < cnt)
goto bounds;

CHECK_VALID(NULL);
memmove(&self->data[dest], &self->data[src], cnt);

Py_RETURN_NONE;
Expand Down Expand Up @@ -847,6 +852,7 @@ mmap_madvise_method(mmap_object *self, PyObject *args)
length = self->size - start;
}

CHECK_VALID(NULL);
if (madvise(self->data + start, length, option) != 0) {
PyErr_SetFromErrno(PyExc_OSError);
return NULL;
Expand Down Expand Up @@ -945,6 +951,7 @@ mmap_subscript(mmap_object *self, PyObject *item)
"mmap index out of range");
return NULL;
}
CHECK_VALID(NULL);
return PyLong_FromLong(Py_CHARMASK(self->data[i]));
}
else if (PySlice_Check(item)) {
Expand All @@ -955,6 +962,7 @@ mmap_subscript(mmap_object *self, PyObject *item)
}
slicelen = PySlice_AdjustIndices(self->size, &start, &stop, step);

CHECK_VALID(NULL);
if (slicelen <= 0)
return PyBytes_FromStringAndSize("", 0);
else if (step == 1)
Expand All @@ -968,6 +976,7 @@ mmap_subscript(mmap_object *self, PyObject *item)

if (result_buf == NULL)
return PyErr_NoMemory();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the buffer have been invalidated here as a side effect of the PyMem_Malloc call above triggering GC? I seem to recall that's possible, but not 100% sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find any GC-related code in Object/obmalloc.c

for (cur = start, i = 0; i < slicelen;
cur += step, i++) {
result_buf[i] = self->data[cur];
Expand Down Expand Up @@ -1052,6 +1061,7 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value)
"in range(0, 256)");
return -1;
}
CHECK_VALID(-1);
self->data[i] = (char) v;
return 0;
}
Expand All @@ -1077,6 +1087,7 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value)
return -1;
}

CHECK_VALID(-1);
if (slicelen == 0) {
}
else if (step == 1) {
Expand Down