Description

cfbolz

opened on Apr 29, 2023

Issue body actions

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

Reacted by sunmy2019

Activity

1. cfbolz

   added

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   on Apr 29, 2023
2. arhadthedev

   added

   extension-modulesC modules in the Modules dirC modules in the Modules dir

   on Apr 29, 2023
3. sobolevn commented on Apr 29, 2023

   

   sobolevn

   on Apr 29, 2023

   Member

   More actions

   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? :)
4. cfbolz commented on Apr 29, 2023

   

   cfbolz

   on Apr 29, 2023

   ContributorAuthor

   More actions

   heh, in principle yes, but I am currently busy doing an equivalent fix in pypy ;-)
5. Agent-Hellboy commented on Apr 29, 2023

   

   Agent-Hellboy

   on Apr 29, 2023

   Contributor

   More actions

   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
6. sobolevn commented on Apr 29, 2023

   

   sobolevn

   on Apr 29, 2023

   Member

   More actions

   @Agent-Hellboy yes, adding regression tests for crashes is a must :)
7. Agent-Hellboy commented on Apr 29, 2023

   

   Agent-Hellboy

   on Apr 29, 2023

   Contributor

   More actions

   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
   
8. sobolevn commented on Apr 29, 2023

   

   sobolevn

   on Apr 29, 2023

   Member

   More actions

   Yes, looks correct to me
9. cfbolz commented on Apr 29, 2023

   

   cfbolz

   on Apr 29, 2023

   ContributorAuthor

   More actions

   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.
10. sobolevn commented on Apr 29, 2023

    

    sobolevn

    on Apr 29, 2023

    Member

    More actions

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

    Reacted by CF Bolz-Tereick
11. Agent-Hellboy

    added a commit that references this issue on Apr 29, 2023

    pythongh-103987: fix crash in mmap module

    d5cf0dd
12. sunmy2019

    added

    3.11only security fixesonly security fixes

    3.12only security fixesonly security fixes

    3.13only security fixesonly security fixes

    on May 12, 2023
13. Agent-Hellboy

    added a commit that references this issue on May 12, 2023

    pythongh-103987: fix crash in mmap module

    6d50192
14. JelleZijlstra

    added a commit that references this issue on May 20, 2023

    gh-103987: fix several crashes in mmap module (#103990)

    ceaa4c3
15. miss-islington

    added a commit that references this issue on May 20, 2023

    pythongh-103987: fix several crashes in mmap module (pythonGH-103990)

    7f358a0
16. JelleZijlstra

    added a commit that references this issue on May 20, 2023

    [3.11] gh-103987: fix several crashes in mmap module (GH-103990) (#10…

    dbb011a
17. carljm

    added a commit that references this issue on May 20, 2023

    Merge branch 'main' into gsallam/main

    ea5965a
18. sobolevn commented on Jun 8, 2023

    

    sobolevn

    on Jun 8, 2023

    Member

    More actions

    Thanks everyone!
19. sobolevn

    closed this as completedon Jun 8, 2023

Issue actions