segfault in mmap object when using __index__ method that closes the mmap

[cfbolz]

The following (artificial) code segfaults CPython (I tried a bunch of versions, including git main) on my x86 Ubuntu Linux 22.10:

import mmap

with open("abcds", "w+") as f:
 f.write("foobar")
 f.flush()

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

 m = mmap.mmap(f.fileno(), 6, access=mmap.ACCESS_READ)

 print(m[1])
 print(m[X()])

The problem is this code in mmapmodule.c

static PyObject *
mmap_subscript(mmap_object *self, PyObject *item)
{
 CHECK_VALID(NULL);
 if (PyIndex_Check(item)) {
 Py_ssize_t i = PyNumber_AsSsize_t(item, PyExc_IndexError);
 if (i == -1 && PyErr_Occurred())
 return NULL;
 if (i < 0)
 i += self->size;
 if (i < 0 || i >= self->size) {
 PyErr_SetString(PyExc_IndexError,
 "mmap index out of range");
 return NULL;
 }
 return PyLong_FromLong(Py_CHARMASK(self->data[i]));
...

the CHECK_VALID(NULL) call which checks whether the mmap object is closed happens before the PyNumber_AsSsize_t call which closes the object (and similarly for the slice handling which happens further down).

Linked PRs

• gh-103987: fix crash in mmap module #103990
• [3.11] gh-103987: fix several crashes in mmap module (GH-103990) #104677

[sobolevn]

Yes, adding CHECK_VALID(NULL); right after Py_ssize_t i = PyNumber_AsSsize_t(item, PyExc_IndexError); seems like a reasonale check.

@cfbolz would you like to send a PR? :)

[cfbolz]

heh, in principle yes, but I am currently busy doing an equivalent fix in pypy ;-)

[Agent-Hellboy]

Hi @sobolevn I can create a PR, I guess it's a one-line change , I don't know whether we need to modify some test or not

[sobolevn]

@Agent-Hellboy yes, adding regression tests for crashes is a must :)

[Agent-Hellboy]

okay, Thanks, let me explore
I guess this is the expected output

Traceback (most recent call last):
 File "/home/proshan/play_python/my_cpython/cpython/b.py", line 15, in <module>
 print(m[X()])
 ~^^^^^
ValueError: mmap closed or invalid

[sobolevn]

Yes, looks correct to me

[cfbolz]

that's what I went with too. there's an interesting corner case with this kind of code:

...
m = mmap.mmap(...)
m.close()
m["abc"]

should this give a TypeError because of the invalid index type? or an error that the mmap is closed (as it does now)? if the latter, you would have to check twice whether the mmap is closed.

[sobolevn]

or an error that the mmap is closed (as it does now)?

I think that's the case, because other exceptions might break users' code (unlikely, but possible).