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

Check usage of Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() in new FASTCALL functions #73492

Closed
vstinner opened this issue Jan 18, 2017 · 13 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

BPO 29306
Nosy @vstinner, @methane
Files
  • enter_recursive_call.patch
  • enter_recursive_call_36.patch
  • check_recursion_depth.py
  • 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 = None
    closed_at = <Date 2018-09-19.23:30:23.704>
    created_at = <Date 2017-01-18.09:04:44.836>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Check usage of Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() in new FASTCALL functions'
    updated_at = <Date 2018-09-19.23:30:23.703>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2018-09-19.23:30:23.703>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-19.23:30:23.704>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2017-01-18.09:04:44.836>
    creator = 'vstinner'
    dependencies = []
    files = ['46327', '46605', '46607']
    hgrepos = []
    issue_num = 29306
    keywords = ['patch']
    message_count = 13.0
    messages = ['285709', '285713', '285727', '287300', '287303', '287305', '287307', '287308', '287314', '287404', '287411', '287412', '325828']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'methane', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29306'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

    @vstinner
    Copy link
    Member Author

    I added the following new functions to Python 3.6:

    • _PyObject_FastCallDict()
    • _PyObject_FastCallKeywords()
    • _PyFunction_FastCallDict()
    • _PyFunction_FastCallKeywords()
    • _PyCFunction_FastCallDict()
    • _PyCFunction_FastCallKeywords()

    I'm not sure that these functions update correctly the "recursion_depth" counter using Py_EnterRecursiveCall() and Py_LeaveRecursiveCall().

    @vstinner vstinner added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jan 18, 2017
    @methane
    Copy link
    Member

    methane commented Jan 18, 2017

    I think basic idea is "wrap unknown function call at least once, and preferably only once".

    _PyEval_EvalFrameDefault() calls Py_EnterRecursiveCall("").
    So wrapping PyFunction_(Fast)Call* with Py_EnterRecursiveCall() seems redundant.

    On the other hand, PyCFunction may calls method of extension module,
    and it can cause recursive call.
    So I think PyCFunction_Call methods calling function pointer it has
    (e.g. _PyCFunction_FastCallKeywords) should call Py_EnterRecursiveCall().

    And caller of them can skip Py_EnterRecursiveCall().
    For example, _PyObject_FastCallDict() may call _PyFunction_FastCallDict() or
    _PyCFunction_FastCallDict(), or tp_fastcall (via _Py_RawFastCallDict) or tp_call.
    It can wrap all, but wrapping only tp_fastcall and tp_call is better.

    @vstinner
    Copy link
    Member Author

    Naoki: "On the other hand, PyCFunction may calls method of extension module, and it can cause recursive call. So I think PyCFunction_Call methods calling function pointer it has (e.g. _PyCFunction_FastCallKeywords) should call Py_EnterRecursiveCall()."

    In Python 3.5, C functions are only calls inside Py_EnterRecursiveCall() if calls from PyObject_Call(). There are many other ways to call C functions which don't use Py_EnterRecursiveCall(). Examples:

    • call_function() => call the C function
    • call_function() => PyCFunction_Call() => call the C function
    • call_function() => do_call() => PyCFunction_Call() => call the C function
    • ext_do_call() => PyCFunction_Call() => call the C function

    call_function() and do_call() have a special case for PyCFunction, otherwise they call the generic PyObject_Call() which uses Py_EnterRecursiveCall().

    I agree that calling C functions with Py_EnterRecursiveCall() would help to prevent stack overflow crashs.

    Attached enter_recursive_call.patch patch:

    • _PyMethodDef_RawFastCallDict() (internal helper of _PyCFunction_FastCallDict()) and _PyCFunction_FastCallKeywords() now use Py_EnterRecursiveCall()
    • _PyObject_FastCallKeywords(): move Py_EnterRecursiveCall() closer to the final call to simplify error handling
    • Modify _PyObject_FastCallDict() to not call _PyFunction_FastCallDict() nor _PyCFunction_FastCallDict() inside Py_EnterRecursiveCall(), since these functions already use Py_EnterRecursiveCall() internally

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2017

    New changeset 88ed9d9eabc1 by Victor Stinner in branch 'default':
    Issue bpo-29306: Fix usage of Py_EnterRecursiveCall()
    https://hg.python.org/cpython/rev/88ed9d9eabc1

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 8, 2017

    I still need to backport fixes to Python 3.6, maybe even Python 3.5.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2017

    New changeset 65d24ff4bbd3320acadb58a5e4d944c84536cb2c by Victor Stinner in branch 'master':
    Issue bpo-29306: Fix usage of Py_EnterRecursiveCall()
    65d24ff

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 8, 2017

    I needed this fix to work on issue bpo-29465. I expected that my patch was reviewed, but woops, it wasn't the case and I missed a refleak. Hopefully, the refleak is now fixed!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2017

    New changeset 37705f89c72b by Victor Stinner in branch 'default':
    Fix refleaks if Py_EnterRecursiveCall() fails
    https://hg.python.org/cpython/rev/37705f89c72b

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2017

    New changeset 1101819ba99afcb4d1b6495d49b17bdd0acfe761 by Victor Stinner in branch 'master':
    Fix refleaks if Py_EnterRecursiveCall() fails
    1101819

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 9, 2017

    enter_recursive_call_36.patch: Patch for Python 3.6.

    • Add Py_EnterRecursiveCall() into C functions
    • Don't call C functions inside a Py_EnterRecursiveCall() block in PyObject_Call()

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 9, 2017

    check_recursion_depth.py: script using Python functions and C function (functools.partial) to check the recursion depth.

    Example with Python 3.6:

    haypo@selma$ ./python check_recursion_depth.py # unpatched
    got: 148, expected: 100

    haypo@selma$ ./python check_recursion_depth.py # patched
    got: 100, expected: 100

    Maybe we need "proxies" in _testcapi for each kind of function, a function taking a callback and calling this function. Function types:

    • C function, METH_NOARGS
    • C function, METH_O
    • C function, METH_VARRGS
    • C function, METH_VARRGS | METH_KEYWORDS
    • C function, METH_FASTCALL
    • Python function
    • Maybe even most common C functions to call functions: PyObject_Call(), _PyObject_FastCallDict(), _PyObject_FastCallKeywords(), etc.

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 9, 2017

    I tested check_recursion_depth.py: Python 2.7, 3.5 and 3.6 have the bug. Python 3.7 is already fixed.

    @vstinner
    Copy link
    Member Author

    I'm not sure about touching the stable branches. At least, the issue has been fixed since Python 3.7. I close the issue.

    @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-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants