Skip to content

Initialize/Finalize Python multiple time and import _ctypes each time lead to a memory corruption #116467

Closed
Closed
Initialize/Finalize Python multiple time and import _ctypes each time lead to a memory corruption#116467
Labels
3.12only security fixes3.13only security fixestopic-ctypestype-crashA hard crash of the interpreter, possibly with a core dump

Description

@vstinner
vstinner
opened on Mar 7, 2024

Example on Windows:

import subprocess
program = r"PCbuild\amd64\_testembed_d.exe"
cmd = [program, "test_repeated_init_exec", "import _ctypes"]

for i in range(1, 11):
    print(f"   == Process #{i} ===")
    proc = subprocess.run(cmd)
    exitcode = proc.returncode
    print(f"=> exitcode {exitcode}")
    if exitcode:
        break
    print()

Output:

vstinner@WIN C:\victor\python\main>python bug.py   
Running Debug|x64 interpreter...
   == Process #1 ===
--- Loop #1 ---
--- Loop #2 ---
--- Loop #3 ---
--- Loop #4 ---
=> exitcode 0

   == Process #2 ===
--- Loop #1 ---
--- Loop #2 ---
Assertion failed: PyUnicode_CheckExact(ep->me_key), file C:\victor\python\main\Objects\dictobject.c, line 922
=> exitcode 3

Activity

  1. serhiy-storchaka
    added
    topic-ctypes
    type-crashA hard crash of the interpreter, possibly with a core dump
    3.13only security fixes
    on Mar 7, 2024

  2. vstinner commented on Mar 7, 2024

    @vstinner
    vstinner
    on Mar 7, 2024
    MemberAuthor

    The regression was introduced by commit ea2c001:

    commit ea2c0016502472aa8baa3149050ada776d17a009
Author: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
Date:   Sat Apr 22 15:39:37 2023 -0400

    gh-84436: Implement Immortal Objects (gh-19474)

    This is the implementation of PEP683

    Motivation:

    The PR introduces the ability to immortalize instances in CPython which bypasses reference counting. Tagging objects as immortal allows up to skip certain operations when we know that the object will be around for the entire execution of the runtime.

    Note that this by itself will bring a performance regression to the runtime due to the extra reference count checks. However, this brings the ability of having truly immutable objects that are useful in other contexts such as immutable data sharing between sub-interpreters.

  3. vstinner commented on Mar 7, 2024

    @vstinner
    vstinner
    on Mar 7, 2024
    MemberAuthor

    _ctypes code can be simplified to:

    static int
_ctypes_mod_exec(PyObject *mod)
{
    ctypes_state *st = GLOBAL_STATE();
    if (PyType_Ready(st->PyCData_Type) < 0) {
        return -1;
    }

    st->PyCStgDict_Type->tp_base = &PyDict_Type;
    if (PyType_Ready(st->PyCStgDict_Type) < 0) {
        return -1;
    }
    return 0;
}


PyMODINIT_FUNC
PyInit__ctypes(void)
{
    PyObject *mod = PyModule_Create(&_ctypesmodule);
    if (!mod) {
        return NULL;
    }

    if (_ctypes_mod_exec(mod) < 0) {
        Py_DECREF(mod);
        return NULL;
    }
    return mod;
}

    I can still reproduce the bug with this code. Maybe the problem comes from the PyCStgDict_Type type which inherits from PyDict_Type (dict type).

  4. vstinner commented on Mar 7, 2024

    @vstinner
    vstinner
    on Mar 7, 2024
    MemberAuthor

    Python 3.12 and main branches are affected (Python 3.11 and older are not affected). I can reproduce the issue on Linux with the script:

    import subprocess
program = r"Programs/_testembed"
cmd = [program, "test_repeated_init_exec", "import _ctypes"]

for i in range(1, 11):
    print(f"   == Process #{i} ===")
    proc = subprocess.run(cmd)
    exitcode = proc.returncode
    print(f"=> exitcode {exitcode}")
    if exitcode:
        break
    print()
  5. serhiy-storchaka
    added
    3.12only security fixes
    on Mar 7, 2024

  6. vstinner commented on Mar 7, 2024

    @vstinner
    vstinner
    on Mar 7, 2024
    MemberAuthor

    The error occurs when the _ctypes extension is imported the second time, so when PyCData_Type type is initialized for the second time by PyType_Ready():

    (...)
#4  0x00007ffff7d1bc57 in __assert_fail () from /lib64/libc.so.6
#5  0x0000000000545fb5 in compare_unicode_unicode (mp=0x0, dk=0x7fffea4cc7c0, ep0=0x7fffea4cc7f0, ix=5, 
    key=0xa6a050 <_PyRuntime+47952>, hash=-4323121235327844407) at Objects/dictobject.c:1101
#6  0x0000000000545c97 in do_lookup (mp=0x0, dk=0x7fffea4cc7c0, key=0xa6a050 <_PyRuntime+47952>, hash=-4323121235327844407, 
    check_lookup=0x545f29 <compare_unicode_unicode>) at Objects/dictobject.c:1027
#7  0x0000000000546032 in unicodekeys_lookup_unicode (dk=0x7fffea4cc7c0, key=0xa6a050 <_PyRuntime+47952>, 
    hash=-4323121235327844407) at Objects/dictobject.c:1112
#8  0x000000000054628e in _Py_dict_lookup (mp=0x7fffea4d4830, key=0xa6a050 <_PyRuntime+47952>, hash=-4323121235327844407, 
    value_addr=0x7fffffff63c8) at Objects/dictobject.c:1201
#9  0x000000000054d354 in _PyDict_Contains_KnownHash (op=0x7fffea4d4830, key=0xa6a050 <_PyRuntime+47952>, 
    hash=-4323121235327844407) at Objects/dictobject.c:4589
#10 0x000000000054d2bf in PyDict_Contains (op=0x7fffea4d4830, key=0xa6a050 <_PyRuntime+47952>) at Objects/dictobject.c:4563
#11 0x000000000059a8cf in _PyType_CheckConsistency (type=0x7ffff7c7e3a0 <PyCData_Type>) at Objects/typeobject.c:597
#12 0x00000000005aa80f in PyType_Ready (type=0x7ffff7c7e3a0 <PyCData_Type>) at Objects/typeobject.c:7844
#13 0x00007ffff7c685f7 in _ctypes_add_types (mod=0x7fffea306930) at ./Modules/_ctypes/_ctypes.c:5582
#14 0x00007ffff7c68ec3 in _ctypes_mod_exec (mod=0x7fffea306930) at ./Modules/_ctypes/_ctypes.c:5709
#15 0x00007ffff7c68f26 in PyInit__ctypes () at ./Modules/_ctypes/_ctypes.c:5728
#16 0x00000000006e93cb in _PyImport_LoadDynamicModuleWithSpec (spec=0x7fffea318380, fp=0x0) at ./Python/importdl.c:170
(...)

(gdb) frame 12
#12 0x00000000005aa80f in PyType_Ready (type=0x7ffff7c7e3a0 <PyCData_Type>) at Objects/typeobject.c:7844
7844	        return 0;
(gdb) p type->tp_name
$6 = 0x7ffff7c75172 "_ctypes._CData"

    At the first PyType_Ready(st->PyCData_Type), PyCData_Type.tp_dict is set to a new dictionary. Dictionary once _ctypes is created:

    {
'__hash__': <slot wrapper '__hash__' of '_ctypes._CData' objects>,
'__buffer__': <slot wrapper '__buffer__' of '_ctypes._CData' objects>,
'__ctypes_from_outparam__': <method '__ctypes_from_outparam__' of '_ctypes._CData' objects>,
'__reduce__': <method '__reduce__' of '_ctypes._CData' objects>,
'__setstate__': <method '__setstate__' of '_ctypes._CData' objects>,
'_b_base_': <member '_b_base_' of '_ctypes._CData' objects>,
'_b_needsfree_': <member '_b_needsfree_' of '_ctypes._CData' objects>,
'_objects': <member '_objects' of '_ctypes._CData' objects>,
'__doc__': 'XXX to be provided',
}

  7. ericsnowcurrently commented on Mar 7, 2024

    @ericsnowcurrently
    ericsnowcurrently
    on Mar 7, 2024
    Member

    CC @eduardo-elizondo

  8. encukou commented on Mar 8, 2024

    @encukou
    encukou
    on Mar 8, 2024
    Member

    I propose removing StgDict, with this big patch: #116458.
    I haven't tested this with that patch yet, but even if it doesn't help, anyone working in this area should be aware of it :)
    It definitely won't help 3.12 of course. Then again, maybe this is too obscure for a fix in 3.12.

  9. erlend-aasland commented on Mar 8, 2024

    @erlend-aasland
    erlend-aasland
    on Mar 8, 2024
    Contributor

    I propose removing StgDict, with this big patch: #116458. I haven't tested this with that patch yet, but even if it doesn't help, anyone working in this area should be aware of it :) It definitely won't help 3.12 of course. Then again,

    Using @vstinner's repro from #116467 (comment), I can confirm that #116458 resolves the issue 🚀 🎉

    IOW, let's get on with reviewing that big patch 😄

  10. neonene commented on Mar 12, 2024

    @neonene
    neonene
    on Mar 12, 2024
    Contributor

    I haven't tested this with that patch yet, but even if it doesn't help,

    #116458 does resolve use-after-free of almost all ctypes-related interned strings.

    Also, please consider making the following string pointers NULL on each init phase, until the module is fully isolated.

    cpython/Tools/c-analyzer/cpython/globals-to-fix.tsv

    Lines 415 to 420 in bb66600

    ##-----------------------
    ## cached - initialized once
    ## manually cached PyUnicodeOjbect
    Modules/_ctypes/callproc.c _ctypes_get_errobj error_object_name -
    Modules/_ctypes/_ctypes.c CreateSwappedType suffix -

  11. erlend-aasland commented on Mar 20, 2024

    @erlend-aasland
    erlend-aasland
    on Mar 20, 2024
    Contributor

    @encukou landed #116458, which also fixes this issue IIUC. Let's close it as resolved. Well, it is fixed in 3.13, but not in 3.12. Correct me if I'm wrong.

  12. vstinner commented on Jun 20, 2024

    @vstinner
    vstinner
    on Jun 20, 2024
    MemberAuthor

    @encukou landed #116458, which also fixes this issue IIUC. Let's close it as resolved. Well, it is fixed in 3.13, but not in 3.12. Correct me if I'm wrong.

    IMO it's ok to not fix the issue in Python 3.12. Apparently, I was the only one who noticed, so it's not a big deal. I close the issue. Thanks for the fix @encukou!

  13. vstinner
    closed this as completedon Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.12only security fixes3.13only security fixestopic-ctypestype-crashA hard crash of the interpreter, possibly with a core dump

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions