Skip to content

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

Closed
Closed
Check usage of Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() in new FASTCALL functions#73492
Labels
3.7 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or error

Description

@vstinner
vstinner
opened on Jan 18, 2017
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']

    Activity

    1. vstinner commented on Jan 18, 2017

      @vstinner
      vstinner
      on Jan 18, 2017
      MemberAuthor

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

    2. vstinner
      added
      3.7 (EOL)end of life
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      type-bugAn unexpected behavior, bug, or error
      on Jan 18, 2017

    3. methane commented on Jan 18, 2017

      @methane
      methane
      on Jan 18, 2017
      Member

      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.

    4. vstinner commented on Jan 18, 2017

      @vstinner
      vstinner
      on Jan 18, 2017
      MemberAuthor

      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

    5. python-dev commented on Feb 8, 2017

      @python-dev
      python-devmannequin
      on Feb 8, 2017
      Mannequin

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

    6. vstinner commented on Feb 8, 2017

      @vstinner
      vstinner
      on Feb 8, 2017
      MemberAuthor

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

    7. python-dev commented on Feb 8, 2017

      @python-dev
      python-devmannequin
      on Feb 8, 2017
      Mannequin

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

    8. vstinner commented on Feb 8, 2017

      @vstinner
      vstinner
      on Feb 8, 2017
      MemberAuthor

      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!

    9. python-dev commented on Feb 8, 2017

      @python-dev
      python-devmannequin
      on Feb 8, 2017
      Mannequin

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

    10. python-dev commented on Feb 8, 2017

      @python-dev
      python-devmannequin
      on Feb 8, 2017
      Mannequin

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

    11. vstinner commented on Feb 9, 2017

      @vstinner
      vstinner
      on Feb 9, 2017
      MemberAuthor

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

    12. vstinner commented on Feb 9, 2017

      @vstinner
      vstinner
      on Feb 9, 2017
      MemberAuthor

      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.

    13. vstinner commented on Feb 9, 2017

      @vstinner
      vstinner
      on Feb 9, 2017
      MemberAuthor

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

    14. vstinner commented on Sep 19, 2018

      @vstinner
      vstinner
      on Sep 19, 2018
      MemberAuthor

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

    15. vstinner
      closed this as completedon Sep 19, 2018
    16. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    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.7 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or error

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions