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.