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

_signal module leak: test_interpreters leaked [1424, 1422, 1424] references #85879

Closed
vstinner opened this issue Sep 4, 2020 · 18 comments
Closed
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Sep 4, 2020

BPO 41713
Nosy @vstinner, @encukou, @shihai1991, @koubaa
PRs
  • bpo-41713: _signal doesn't use multi-phase init #22087
  • [WIP] bpo-41713: Move _signal variables into a state structure #23318
  • bpo-41713: Remove PyOS_InitInterrupts() function #23342
  • bpo-41686: Always create the SIGINT event on Windows #23344
  • [3.9] bpo-41686: Always create the SIGINT event on Windows (GH-23344) #23347
  • [3.8] bpo-41686: Always create the SIGINT event on Windows (GH-23344) (GH-23347) #23349
  • bpo-41713: Port _signal module to multi-phase init #23355
  • bpo-41713: Remove PyOS_InitInterrupts() from python3dll.c #24257
  • 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 2020-11-17.22:29:15.934>
    created_at = <Date 2020-09-04.09:53:41.602>
    labels = ['interpreter-core', '3.10']
    title = '_signal module leak: test_interpreters leaked [1424, 1422, 1424] references'
    updated_at = <Date 2021-04-27.23:34:44.792>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-04-27.23:34:44.792>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-11-17.22:29:15.934>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2020-09-04.09:53:41.602>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41713
    keywords = ['patch']
    message_count = 18.0
    messages = ['376345', '376348', '376349', '376359', '376364', '376469', '376498', '381246', '381258', '381267', '381287', '381293', '381298', '381299', '385268', '385275', '385280', '392153']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'petr.viktorin', 'shihai1991', 'koubaa']
    pr_nums = ['22087', '23318', '23342', '23344', '23347', '23349', '23355', '24257']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue41713'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 4, 2020

    AMD64 Fedora Rawhide Refleaks 3.x:
    https://buildbot.python.org/all/#/builders/565/builds/11

    test_interpreters leaked [1424, 1422, 1424] references, sum=4270

    According to git bisect, the leak was introduced by:

    commit 71d1bd9
    Author: Mohamed Koubaa <koubaa.m@gmail.com>
    Date: Thu Sep 3 03:21:06 2020 -0500

    bpo-1635741: Port _signal module to multi-phase init (PEP-489) (GH-22049)
    

    .../2020-09-01-17-07-20.bpo-1635741.7wSuCc.rst | 1 +
    Modules/signalmodule.c | 168 +++++++++++----------
    2 files changed, 87 insertions(+), 82 deletions(-)

    Example of leak:

    $ ./python -m test -R 3:3 test_interpreters -m test.test_interpreters.TestInterpreterClose.test_from_current
    0:00:00 load avg: 0.72 Run tests sequentially
    0:00:00 load avg: 0.72 [1/1] test_interpreters
    beginning 6 repetitions
    123456
    ......
    test_interpreters leaked [237, 237, 237] references, sum=711
    test_interpreters leaked [18, 18, 18] memory blocks, sum=54
    test_interpreters failed

    == Tests result: FAILURE ==

    1 test failed:
    test_interpreters

    Total duration: 1.1 sec
    Tests result: FAILURE

    @vstinner vstinner added 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 4, 2020
    @vstinner vstinner changed the title test_interpreters leaked [1424, 1422, 1424] references _signal module leak: test_interpreters leaked [1424, 1422, 1424] references Sep 4, 2020
    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 4, 2020

    The signal module is really special. In Python, only the main thread of the main interpreter is supposed to be allowed to run Python signal handlers (but the C signal handler can be executed by any thread).

    The exec function of the _signal module runs actions each time a new instance of the _signal module is created, whereas some actions must only be done exactly once:

    • modify Handlers[i].tripped
    • modify Handlers[i].func
    • Set SIGINT signal handler
    • Initialize sigint_event event (Windows only): see bpo-41686

    For example, calling signal.signal() in a subinterpreter raises ValueError("signal only works in main thread of the main interpreter").

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 4, 2020

    About the leak, the following three variables are also initialized multiple times by signal_exec():

    static PyObject *DefaultHandler;
    static PyObject *IgnoreHandler;
    static PyObject *IntHandler;

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 4, 2020

    Another problem: PyOS_FiniInterrupts() is a public function of the C API which access global variables like IntHandler, DefaultHandler and IgnoreHandler.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 4, 2020

    New changeset 4b8032e by Victor Stinner in branch 'master':
    bpo-41713: _signal doesn't use multi-phase init (GH-22087)
    4b8032e

    @koubaa
    Copy link
    Mannequin

    koubaa mannequin commented Sep 6, 2020

    Sounds like there needs to be some python-wide global state that perhaps the signal module wraps rather than owns. I could try to prototype it if you agree

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 7, 2020

    See also:

    • bpo-40600: "Add an option to disallow creating more than one instance of a module".
    • bpo-40288: [subinterpreters] atexit module should not be loaded more than once per interpreter

    @vstinner
    Copy link
    Member Author

    New changeset 296a796 by Victor Stinner in branch 'master':
    bpo-41713: Remove PyOS_InitInterrupts() function (GH-23342)
    296a796

    @vstinner
    Copy link
    Member Author

    New changeset 0ae323b by Victor Stinner in branch 'master':
    bpo-41686: Always create the SIGINT event on Windows (GH-23344)
    0ae323b

    @vstinner
    Copy link
    Member Author

    New changeset 05a5d69 by Victor Stinner in branch '3.9':
    bpo-41686: Always create the SIGINT event on Windows (GH-23344) (GH-23347)
    05a5d69

    @vstinner
    Copy link
    Member Author

    New changeset a702bd4 by Victor Stinner in branch '3.8':
    bpo-41686: Always create the SIGINT event on Windows (GH-23344) (GH-23347) (GH-23349)
    a702bd4

    @vstinner
    Copy link
    Member Author

    New changeset 29aa624 by Victor Stinner in branch 'master':
    bpo-41686: Move _Py_RestoreSignals() to signalmodule.c (GH-23353)
    29aa624

    @vstinner
    Copy link
    Member Author

    New changeset 7f9b25a by Victor Stinner in branch 'master':
    bpo-41713: Port _signal module to multi-phase init (GH-23355)
    7f9b25a

    @vstinner
    Copy link
    Member Author

    Done! _signal uses again the multi-phase init API.

    @encukou
    Copy link
    Member

    encukou commented Jan 19, 2021

    The PyOS_InitInterrupts function is still listed in PC/python3dll.c.

    @vstinner
    Copy link
    Member Author

    Petr Viktorin:

    The PyOS_InitInterrupts function is still listed in PC/python3dll.c.

    Ooops, I proposed PR 24257 to remove it.

    @vstinner
    Copy link
    Member Author

    New changeset e8e66ea by Victor Stinner in branch 'master':
    bpo-41713: Remove PyOS_InitInterrupts() from python3dll.c (GH-24257)
    e8e66ea

    @vstinner
    Copy link
    Member Author

    bpo-41713: Port _signal module to multi-phase init (GH-23355)

    This change caused a regression: bpo-43963.

    @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.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants