Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

co_extra_freefuncs is stored thread locally and can lead to crashes #74789

Closed
DinoV opened this issue Jun 8, 2017 · 21 comments
Closed

co_extra_freefuncs is stored thread locally and can lead to crashes #74789

DinoV opened this issue Jun 8, 2017 · 21 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@DinoV
Copy link
Contributor

DinoV commented Jun 8, 2017

BPO 30604
Nosy @brettcannon, @vstinner, @ned-deily, @DinoV, @serhiy-storchaka, @1st1, @Mariatta, @jeethu
PRs
  • bpo-30604: Move co_extra_freefuncs to interpreter state to avoid crashes in threads #2015
  • bpo-30604: clean up co_extra support #2144
  • [3.6] bpo-30604: Fix __PyCodeExtraState_Get() prototype #2152
  • bpo-30604: Skip CoExtra tests if ctypes is missing #2356
  • [3.6] bpo-30604: Skip CoExtra tests if ctypes is missing (#2356) #2358
  • bpo-30704, bpo-30604: Fix memleak in code_dealloc() #2455
  • [3.6] bpo-30704, bpo-30604: Fix memleak in code_dealloc() (#2455) #2456
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/DinoV'
    closed_at = <Date 2017-06-23.13:26:09.274>
    created_at = <Date 2017-06-08.21:37:03.405>
    labels = ['interpreter-core', '3.7', 'type-crash']
    title = 'co_extra_freefuncs is stored thread locally and can lead to crashes'
    updated_at = <Date 2018-01-17.02:43:32.298>
    user = 'https://github.com/DinoV'

    bugs.python.org fields:

    activity = <Date 2018-01-17.02:43:32.298>
    actor = 'jeethu'
    assignee = 'dino.viehland'
    closed = True
    closed_date = <Date 2017-06-23.13:26:09.274>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2017-06-08.21:37:03.405>
    creator = 'dino.viehland'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30604
    keywords = ['patch']
    message_count = 21.0
    messages = ['295467', '295477', '295489', '295835', '295845', '295868', '296301', '296588', '296595', '296596', '296597', '296599', '296607', '296709', '296718', '296719', '297076', '297078', '297079', '297082', '297938']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'vstinner', 'ned.deily', 'dino.viehland', 'serhiy.storchaka', 'yselivanov', 'Mariatta', 'jeethu']
    pr_nums = ['2015', '2144', '2152', '2356', '2358', '2455', '2456']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue30604'
    versions = ['Python 3.6', 'Python 3.7']

    @DinoV
    Copy link
    Contributor Author

    DinoV commented Jun 8, 2017

    The co_extra_freefuncs are stored in PyThreadState. When calling _PyEval_RequestCodeExtraIndex you are given a thread specific index. The code object can then lose it's last reference on a different thread, and the wrong free function can be called if users of the extra space have made calls to get their index in different orders.

    This can also lead to crashes if the extra thread hasn't yet requested extra indexes either.

    @DinoV DinoV self-assigned this Jun 8, 2017
    @DinoV DinoV added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Jun 8, 2017
    @ned-deily
    Copy link
    Member

    See review comments on PR 2015.

    @ned-deily ned-deily added the 3.7 (EOL) end of life label Jun 9, 2017
    @serhiy-storchaka
    Copy link
    Member

    User code shouldn't allocate PyInterpreterState and PyThreadState structures, it only uses structures created by the interpreter. Changing the size of PyInterpreterState should be safe. The only possible breaking compatibility is if user code directly access co_extra_user_count, co_extra_freefuncs, async_gen_firstiter or async_gen_finalizer rather than using the API: _PyEval_RequestCodeExtraIndex(), _PyCode_GetExtra(), _PyCode_SetExtra(), _PyEval_GetAsyncGenFirstiter(), _PyEval_SetAsyncGenFirstiter(), _PyEval_GetAsyncGenFinalizer() and _PyEval_SetAsyncGenFinalizer().

    Nick's idea about _preserve_36_ABI_1 and _preserve_36_ABI_2 should address concerns about direct access to async_gen_firstiter and async_gen_finalizer. Direct access to co_extra_user_count and co_extra_freefuncs obviously can't be preserved.

    PyInterpreterState and PyThreadState are not in the stable ABI. They are opaque types when use limited API. May be they should be made opaque for all user code (in 3.7+).

    @ned-deily
    Copy link
    Member

    New changeset 2997fec by Ned Deily (Dino Viehland) in branch '3.6':
    [3.6] bpo-30604: Move co_extra_freefuncs to interpreter state to avoid crashes in threads (bpo-2015)
    2997fec

    @serhiy-storchaka
    Copy link
    Member

    Avoid using double underscores in C code. C compiler uses names with double underscores for its own needs, and this can lead to conflicts.

    @vstinner
    Copy link
    Member

    New changeset 932946c by Victor Stinner in branch '3.6':
    bpo-30604: Fix __PyCodeExtraState_Get() prototype (bpo-2152)
    932946c

    @Mariatta
    Copy link
    Member

    I'm setting the stage to 'backport needed', but it really is a 'porting needed' stage :)

    The two PRs merged PRs here were made against 3.6.

    @1st1
    Copy link
    Member

    1st1 commented Jun 21, 2017

    New changeset f3cffd2 by Yury Selivanov (Dino Viehland) in branch 'master':
    bpo-30604: clean up co_extra support (bpo-2144)
    f3cffd2

    @brettcannon
    Copy link
    Member

    Should this be closed since the all PRs got merged?

    @Mariatta
    Copy link
    Member

    PR 2152 is not yet ported to master.

    @1st1
    Copy link
    Member

    1st1 commented Jun 21, 2017

    It doesn't need to be, it's only for 3.6

    @1st1
    Copy link
    Member

    1st1 commented Jun 21, 2017

    Closing the issue. Thank you Dino for working on this!

    @1st1 1st1 closed this as completed Jun 21, 2017
    @vstinner
    Copy link
    Member

    If a test requires ctypes, please skip your test if ctypes is missing. The new test fails on the "x86 Ubuntu Shared 3.x" buildbot which lacks the _ctypes module (for an unknown reason, but does it really matter here? ;-)).

    http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/924/steps/test/logs/stdio

    test test_code crashed -- Traceback (most recent call last):
      File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/libregrtest/runtest.py", line 156, in runtest_inner
        the_module = importlib.import_module(abstest)
      File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/importlib/__init__.py", line 127, in import_module
        return _bootstrap._gcd_import(name[level:], package, level)
      File "<frozen importlib._bootstrap>", line 978, in _gcd_import
      File "<frozen importlib._bootstrap>", line 961, in _find_and_load
      File "<frozen importlib._bootstrap>", line 950, in _find_and_load_unlocked
      File "<frozen importlib._bootstrap>", line 655, in _load_unlocked
      File "<frozen importlib._bootstrap_external>", line 679, in exec_module
      File "<frozen importlib._bootstrap>", line 205, in _call_with_frames_removed
      File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_code.py", line 218, in <module>
        import ctypes
      File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/ctypes/__init__.py", line 7, in <module>
        from _ctypes import Union, Structure, Array
    ModuleNotFoundError: No module named '_ctypes'

    @vstinner vstinner reopened this Jun 22, 2017
    @vstinner
    Copy link
    Member

    New changeset a4b091e by Victor Stinner in branch 'master':
    bpo-30604: Skip CoExtra tests if ctypes is missing (bpo-2356)
    a4b091e

    @vstinner
    Copy link
    Member

    New changeset cea2174 by Victor Stinner in branch '3.6':
    bpo-30604: Skip CoExtra tests if ctypes is missing (bpo-2356) (bpo-2358)
    cea2174

    @vstinner
    Copy link
    Member

    The test_code is fixed again, so I close the issue.

    @vstinner
    Copy link
    Member

    _PyCode_SetExtra() uses two memory block for code extras. By changing how memory is accessed and allocated, it would be possible to use a single memory block. Was it on purpose to use two memory blocks?

    See for example PyTupleObject which uses a single memory block vs PyListObject which uses two memory blocks.

    typedef struct {
        PyObject_VAR_HEAD
        PyObject *ob_item[1];
    /* ob_item contains space for 'ob_size' elements.
     * Items must normally not be NULL, except during construction when
     * the tuple is not yet visible outside the function that builds it.
     \*/
    

    } PyTupleObject;

    @vstinner
    Copy link
    Member

    _PyCode_SetExtra() uses two memory block for code extras. By changing how memory is accessed and allocated, it would be possible to use a single memory block. Was it on purpose to use two memory blocks?

    I discussed with Yury who is not opposed to such change in Python 3.7, so I created bpo-30789.

    @vstinner
    Copy link
    Member

    New changeset 23e7944 by Victor Stinner in branch 'master':
    bpo-30704, bpo-30604: Fix memleak in code_dealloc() (bpo-2455)
    23e7944

    @vstinner
    Copy link
    Member

    New changeset 26daad4 by Victor Stinner in branch '3.6':
    bpo-30704, bpo-30604: Fix memleak in code_dealloc() (bpo-2455) (bpo-2456)
    26daad4

    @ned-deily
    Copy link
    Member

    New changeset c794b64 by Ned Deily (Victor Stinner) in branch '3.6':
    bpo-30704, bpo-30604: Fix memleak in code_dealloc() (bpo-2455) (bpo-2456)
    c794b64

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants