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() withself.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

elseif (PySlice_Check(item)){Py_ssize_tstart, stop, step, slicelen; if (PySlice_Unpack(item, &start, &stop, &step) <0){returnNULL} CHECK_VALID(NULL); /////////////////////////////////////////////////////////////////slicelen=PySlice_AdjustIndices(self->size, &start, &stop, step); if (slicelen <= 0) returnPyBytes_FromStringAndSize("", 0); elseif (step==1) returnPyBytes_FromStringAndSize(self->data+start, slicelen); else{char*result_buf= (char*)PyMem_Malloc(slicelen); size_tcur; Py_ssize_ti; PyObject*result; if (result_buf==NULL) returnPyErr_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); returnresult} } else{PyErr_SetString(PyExc_TypeError, "mmap indices must be integers"); returnNULL} }

[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.