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

Jump to bottom

Possible reference leak in error paths of update_bases() and __build_class__ #89019

Closed
pablogsal opened this issue Aug 7, 2021 · 7 comments
Closed

Possible reference leak in error paths of update_bases() and __build_class__ #89019

pablogsal opened this issue Aug 7, 2021 · 7 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes

Comments

@pablogsal
Copy link
Member

BPO 44856
Nosy @ambv, @pablogsal, @miss-islington
PRs
  • bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ #27647
  • [3.9] bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647) #27651
  • [3.8] bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647) #27652
  • [3.10] bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647) #27653
    		•

    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 2021-08-07.14:12:16.906>
created_at = <Date 2021-08-07.00:25:24.565>
labels = ['3.8', '3.9', '3.10', '3.11']
title = 'Possible reference leak in error paths of update_bases() and __build_class__'
updated_at = <Date 2021-08-07.14:12:16.905>
user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2021-08-07.14:12:16.905>
actor = 'lukasz.langa'
assignee = 'none'
closed = True
closed_date = <Date 2021-08-07.14:12:16.906>
closer = 'lukasz.langa'
components = []
creation = <Date 2021-08-07.00:25:24.565>
creator = 'pablogsal'
dependencies = []
files = []
hgrepos = []
issue_num = 44856
keywords = ['patch']
message_count = 7.0
messages = ['399161', '399163', '399172', '399174', '399175', '399180', '399182']
nosy_count = 3.0
nosy_names = ['lukasz.langa', 'pablogsal', 'miss-islington']
pr_nums = ['27647', '27651', '27652', '27653']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44856'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']
    @pablogsal
    Copy link
    Member Author

    Here:

    cpython/Python/bltinmodule.c

    Lines 60 to 88 in 17c2316

    new_base = PyObject_CallOneArg(meth, bases);
    Py_DECREF(meth);
    if (!new_base) {
    goto error;
    }
    if (!PyTuple_Check(new_base)) {
    PyErr_SetString(PyExc_TypeError,
    "__mro_entries__ must return a tuple");
    Py_DECREF(new_base);
    goto error;
    }
    if (!new_bases) {
    /* If this is a first successful replacement, create new_bases list and
    copy previously encountered bases. */
    if (!(new_bases = PyList_New(i))) {
    goto error;
    }
    for (j = 0; j < i; j++) {
    base = args[j];
    PyList_SET_ITEM(new_bases, j, base);
    Py_INCREF(base);
    }
    }
    j = PyList_GET_SIZE(new_bases);
    if (PyList_SetSlice(new_bases, j, j, new_base) < 0) {
    goto error;
    }
    Py_DECREF(new_base);
    }

    Seems that new_base is not properly cleaned on the error paths

    @pablogsal pablogsal added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Aug 7, 2021
    @pablogsal
    Copy link
    Member Author

    Oh, damn, turns out that there is a ton of reference leaks in the error path of __build_class__ as well.

    These leaks have been since forever!

    Curiously I found about them when debugging https://bugs.python.org/issue44524

    @pablogsal pablogsal changed the title Possible reference leak in error paths of update_bases() Possible reference leak in error paths of update_bases() and __build_class__ Aug 7, 2021
    @pablogsal pablogsal changed the title Possible reference leak in error paths of update_bases() Possible reference leak in error paths of update_bases() and __build_class__ Aug 7, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Aug 7, 2021

    New changeset a40675c by Pablo Galindo Salgado in branch 'main':
    bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647)
    a40675c

    @ambv
    Copy link
    Contributor

    ambv commented Aug 7, 2021

    New changeset 0a42309 by Miss Islington (bot) in branch '3.8':
    bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647) (GH-27652)
    0a42309

    @ambv
    Copy link
    Contributor

    ambv commented Aug 7, 2021

    New changeset ed718e9 by Miss Islington (bot) in branch '3.9':
    bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647) (GH-27651)
    ed718e9

    @miss-islington
    Copy link
    Contributor

    New changeset ac8f72c by Miss Islington (bot) in branch '3.10':
    bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ (GH-27647)
    ac8f72c

    @ambv
    Copy link
    Contributor

    ambv commented Aug 7, 2021

    Confirmed this fixed refleaks in test_typing. Backported to all branches listed on the issue.

    Thanks for super-effective debugging and the fix, Pablo!

    @ambv ambv closed this as completed Aug 7, 2021
    @ambv ambv closed this as completed Aug 7, 2021
    @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
    Assignees
    No one assigned
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @ambv @pablogsal @miss-islington