gh-103987: fix crash in mmap module

[Agent-Hellboy]

• Issue: segfault in mmap object when using __index__ method that closes the mmap #103987

[Agent-Hellboy]

I am closing the mmap object, still the test is failing

[Eclips4]

Let's wait CI/CD results. Also, can you add a few lines about this in NEWS entry?

[Eclips4]

Great work @Agent-Hellboy !
Let's see what core devs will say.

[cfbolz]

still some cases not covered properly. Now that you moved the CHECK_VALID(NULL); into the if-branch, you don't check whether the mmap is closed any more if the index is a slice. try adding a test like:

m = mmap(...) m.close() m[0:5] # should complain about a closed file

also you should add a test that uses the weird X class as part of a slice

m[X():5]

and you need to do the same thing again for assignment, both with X as an index and a slice:

m[X()] = 1 ... m[X():5] = b'abcde'

or something like that.

Also there needs to be a test for the special case I mentioned at the end of the issue:

m = mmap(...) m.close() with self.assertRaises(ValueError): m["abc"] # not TypeError!

[Agent-Hellboy]

hi @cfbolz, Now I am checking at the start as well which denies throwing TypeError for closed mmap

[cfbolz]

now you need to do the same things for item assignments still.

[sunmy2019]

We should CHECK_VALID(NULL); before each self->data access, unless we have strong proof that our handle is not closed.

[sunmy2019]

@Agent-Hellboy

I am saying you should add these

 else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return NULL; } CHECK_VALID(NULL); ///////////////////////////////////////////////////////////////// slicelen = PySlice_AdjustIndices(self->size, &start, &stop, step); if (slicelen <= 0) return PyBytes_FromStringAndSize("", 0); else if (step == 1) return PyBytes_FromStringAndSize(self->data + start, slicelen); else { char *result_buf = (char *)PyMem_Malloc(slicelen); size_t cur; Py_ssize_t i; PyObject *result; if (result_buf == NULL) return PyErr_NoMemory(); CHECK_VALID(NULL); ///////////////////////////////////////////////////////////////// for (cur = start, i = 0; i < slicelen; cur += step, i++) { result_buf[i] = self->data[cur]; } result = PyBytes_FromStringAndSize(result_buf, slicelen); PyMem_Free(result_buf); return result; } } else { PyErr_SetString(PyExc_TypeError, "mmap indices must be integers"); return NULL; } }

[Agent-Hellboy]

Hi @sunmy2019, please share with me a scenario where it's bypassing the first CHECK_VALID(NULL); at the start for a slice

[sunmy2019]

Hi @sunmy2019, please share with me a scenario where it's bypassing the first CHECK_VALID(NULL); at the start for a slice

m[X():5]

[Agent-Hellboy]

Currently, it's throwing expected Value error, i have added a test for the same

[sunmy2019]

Currently, it's throwing expected Value error

This is wrong. And you are not creating a new m, which covered your mistake.

[sunmy2019]

then we access self->fd

I think checking self->fd != -1 is enough here.

[sunmy2019]

I think it's now ready for review. @JelleZijlstra @cfbolz

Sorry for the delay!

[JelleZijlstra]

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.

[sunmy2019]

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

[miss-islington]

Thanks @Agent-Hellboy for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[bedevere-bot]

GH-104677 is a backport of this pull request to the 3.11 branch.