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.