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

reference counter issue in signal module #82218

Closed
animalize mannequin opened this issue Sep 5, 2019 · 7 comments
Closed

reference counter issue in signal module #82218

animalize mannequin opened this issue Sep 5, 2019 · 7 comments
Labels
3.8 only security fixes 3.9 only security fixes release-blocker

Comments

@animalize
Copy link
Mannequin

animalize mannequin commented Sep 5, 2019

BPO 38037
Nosy @vstinner, @ambv, @berkerpeksag, @animalize, @miss-islington, @nanjekyejoannah
PRs
  • bpo-38037: Fix reference counters in signal module #15701
  • bpo-38037: Fix reference counters in signal module #15753
  • [3.8] bpo-38037: Fix reference counters in signal module (GH-15753) #15779
  • Files
  • reproduce.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 2019-09-09.14:44:15.832>
    created_at = <Date 2019-09-05.11:14:08.483>
    labels = ['3.8', '3.9', 'release-blocker']
    title = 'reference counter issue in signal module'
    updated_at = <Date 2019-09-09.14:44:15.832>
    user = 'https://github.com/animalize'

    bugs.python.org fields:

    activity = <Date 2019-09-09.14:44:15.832>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-09-09.14:44:15.832>
    closer = 'vstinner'
    components = []
    creation = <Date 2019-09-05.11:14:08.483>
    creator = 'malin'
    dependencies = []
    files = ['48594']
    hgrepos = []
    issue_num = 38037
    keywords = ['patch']
    message_count = 7.0
    messages = ['351196', '351200', '351249', '351250', '351466', '351483', '351485']
    nosy_count = 6.0
    nosy_names = ['vstinner', 'lukasz.langa', 'berker.peksag', 'malin', 'miss-islington', 'nanjekyejoannah']
    pr_nums = ['15701', '15753', '15779']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue38037'
    versions = ['Python 3.8', 'Python 3.9']

    @animalize
    Copy link
    Mannequin Author

    animalize mannequin commented Sep 5, 2019

    Adding these two lines to /Objects/longobject.c will disable the "preallocated small integer pool":

        #define NSMALLPOSINTS  0
        #define NSMALLNEGINTS  0

    Then run this reproduce code (attached):

        from enum import IntEnum
        import _signal
    
        class Handlers(IntEnum):
            A = _signal.SIG_DFL
            B = _signal.SIG_IGN

    When the interpreter exits, will get this error:

    d:\dev\cpython\PCbuild\win32>python_d.exe d:\a.py
    d:\dev\cpython\include\object.h:541: _Py_NegativeRefcount: Assertion failed: object has negative ref count
    <object: freed>
    Fatal Python error: _PyObject_AssertFailed
    
    Current thread 0x0000200c (most recent call first):
    

    3.8 and 3.9 branches are affected.
    I'm sorry, this issue is beyond my ability.

    @animalize animalize mannequin added 3.8 only security fixes 3.9 only security fixes labels Sep 5, 2019
    @animalize
    Copy link
    Mannequin Author

    animalize mannequin commented Sep 5, 2019

    I did a Git bisect, this is the first bad commit:
    9541bd3

    nosy involved mates.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 6, 2019

    This issue is a Python 3.8 regression.

    Joannah: Would you mind to have a look?

        x = DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
        if (PyModule_AddObject(m, "SIG_DFL", x))
            goto finally;

    This change is not easy to read.

    DefaultHandler must be a strong reference: finisignal() calls Py_CLEAR(DefaultHandler);

    Previously, the code uses PyDict_SetItemString(d, "SIG_DFL", x): PyDict_SetItemString increases the reference counter, whereas PyModule_AddObject leaves the reference counter unchanged (yeah, it's a strange/bad C API).

    I guess than an INCREF() is needed somewhere.

    Compare it to:

    int
    PyModule_AddIntConstant(PyObject *m, const char *name, long value)
    {
        PyObject *o = PyLong_FromLong(value);
        if (!o)
            return -1;
        if (PyModule_AddObject(m, name, o) == 0)
            return 0;
        Py_DECREF(o);
        return -1;
    }

    @nanjekyejoannah
    Copy link
    Member

    I will look after my next meeting which is in 10 minutes. In the meantime have you perused this PR : #15701 If it can solve this?

    @animalize animalize mannequin changed the title Assertion failed: object has negative ref count reference counter issue in signal module Sep 7, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Sep 9, 2019

    New changeset 77643c4 by Victor Stinner (animalize) in branch 'master':
    bpo-38037: Fix reference counters in signal module (GH-15753)
    77643c4

    @miss-islington
    Copy link
    Contributor

    New changeset b150d0b by Miss Islington (bot) in branch '3.8':
    bpo-38037: Fix reference counters in signal module (GH-15753)
    b150d0b

    @vstinner
    Copy link
    Member

    vstinner commented Sep 9, 2019

    Thanks for the fix animalize.

    @vstinner vstinner closed this as completed Sep 9, 2019
    @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.8 only security fixes 3.9 only security fixes release-blocker
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants