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

memory leak in PyEval_EvalCodeEx #90505

Closed
1st1 opened this issue Jan 11, 2022 · 18 comments
Closed

memory leak in PyEval_EvalCodeEx #90505

1st1 opened this issue Jan 11, 2022 · 18 comments
Labels
3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@1st1
Copy link
Member

1st1 commented Jan 11, 2022

BPO 46347
Nosy @gpshead, @vstinner, @larryhastings, @elprans, @markshannon, @1st1, @pablogsal, @miss-islington
PRs
  • bpo-46347: Fix memory leak in PyEval_EvalCodeEx. #30546
  • [3.10] bpo-46347: Fix memory leak in PyEval_EvalCodeEx. (GH-30546) #30549
  • bpo-46347: Fix PyEval_EvalCodeEx to correctly cleanup in error paths #30551
  • [3.10] bpo-46347: Fix PyEval_EvalCodeEx to correctly cleanup in error paths #30553
  • bpo-46347: Yet another fix in the erorr path of PyEval_EvalCodeEx #30554
    		•

    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 2022-01-11.23:38:20.113>
created_at = <Date 2022-01-11.21:06:10.203>
labels = ['interpreter-core', '3.10', '3.11']
title = 'memory leak in PyEval_EvalCodeEx'
updated_at = <Date 2022-01-12.02:03:34.304>
user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2022-01-12.02:03:34.304>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2022-01-11.23:38:20.113>
closer = 'yselivanov'
components = ['Interpreter Core']
creation = <Date 2022-01-11.21:06:10.203>
creator = 'yselivanov'
dependencies = []
files = []
hgrepos = []
issue_num = 46347
keywords = ['patch']
message_count = 18.0
messages = ['410329', '410330', '410331', '410332', '410341', '410342', '410343', '410344', '410345', '410349', '410352', '410354', '410358', '410368', '410371', '410375', '410376', '410380']
nosy_count = 8.0
nosy_names = ['gregory.p.smith', 'vstinner', 'larry', 'Elvis.Pranskevichus', 'Mark.Shannon', 'yselivanov', 'pablogsal', 'miss-islington']
pr_nums = ['30546', '30549', '30551', '30553', '30554']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46347'
versions = ['Python 3.10', 'Python 3.11']
    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    I'm investigating a memory leak in 3.10, and while looking at the offending commit I stumbled upon this: in ceval.c:PyEval_EvalCodeEx, there's this allocation:

        PyObject **kwargs = PyMem_Malloc(sizeof(PyObject *)*kwcount);

    The problem is that this isn't ever freed. And kwargs isn't used anywhere in the function body. It seems to me that this is silently leaking memory.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    (this is the context of our current investigation btw: MagicStack/asyncpg#874 (comment))

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    This is the questionable place in the code:

    cpython/Python/ceval.c

    Lines 6131 to 6166 in dce642f

    PyObject **kwargs = PyMem_Malloc(sizeof(PyObject *)*kwcount);
    if (kwargs == NULL) {
    res = NULL;
    Py_DECREF(kwnames);
    goto fail;
    }
    for (int i = 0; i < kwcount; i++) {
    Py_INCREF(kws[2*i]);
    PyTuple_SET_ITEM(kwnames, i, kws[2*i]);
    kwargs[i] = kws[2*i+1];
    }
    PyFrameConstructor constr = {
    .fc_globals = globals,
    .fc_builtins = builtins,
    .fc_name = ((PyCodeObject *)_co)->co_name,
    .fc_qualname = ((PyCodeObject *)_co)->co_name,
    .fc_code = _co,
    .fc_defaults = defaults,
    .fc_kwdefaults = kwdefs,
    .fc_closure = closure
    };
    PyFunctionObject *func = _PyFunction_FromConstructor(&constr);
    if (func == NULL) {
    return NULL;
    }
    res = _PyEval_Vector(tstate, func, locals,
    allargs, argcount,
    kwnames);
    Py_DECREF(func);
    if (kwcount) {
    Py_DECREF(kwnames);
    PyMem_Free(newargs);
    }
    fail:
    Py_DECREF(defaults);
    return res;

    See that kwargs is allocated but never freed or used.

    @elprans
    Copy link
    Mannequin

    elprans mannequin commented Jan 11, 2022

    I can confirm that removing the kwargs allocation prevents the leak described in the asyncpg issue from happening.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    New changeset 607d8a8 by Yury Selivanov in branch 'main':
    bpo-46347: Fix memory leak in PyEval_EvalCodeEx. (bpo-30546)
    607d8a8

    @pablogsal
    Copy link
    Member

    Turns out this leak affects CYthon quite a lot:

    https://github.com/cython/cython/blob/29ad96444b8b1a4f05a6ac2328fde01de4782691/Cython/Utility/ObjectHandling.c#L2139-L2155

    This seems to imply that every function call using __Pyx_PyCFunction_FastCall is leaking memory in 3.10, which is quite bad. We may need an urgent release for this :(

    @larryhastings
    Copy link
    Contributor

    The function will still leak "kwnames" and "default" if creating the "func" object fails. Admittedly that would only happen in a low-memory condition which is a) rare and b) probably only happens just before the interpreter completely dies, so it's not worth addressing during today's mild emergency.

    @larryhastings
    Copy link
    Contributor

    (Sorry--it'll leak "kwnames", "newargs", and "defaults".)

    @pablogsal
    Copy link
    Member

    Apparently, this is causing adyncpg to leak megabytes in seconds

    @miss-islington
    Copy link
    Contributor

    New changeset b1a94f1 by Miss Islington (bot) in branch '3.10':
    bpo-46347: Fix memory leak in PyEval_EvalCodeEx. (GH-30546)
    b1a94f1

    @vstinner
    Copy link
    Member

    The memory leak has been fixed in 3.10 and main branches. Can this issue be closed now?

    @vstinner vstinner added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.10 only security fixes 3.11 only security fixes labels Jan 11, 2022
    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    The function will still leak "kwnames" and "default" if creating the "func" object fails.

    Yeah, here's another PR to address that: #30551

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2022

    New changeset 20b5791 by Yury Selivanov in branch 'main':
    bpo-46347: Fix PyEval_EvalCodeEx to correctly cleanup in error paths (bpo-30551)
    20b5791

    @1st1 1st1 closed this as completed Jan 11, 2022
    @1st1 1st1 closed this as completed Jan 11, 2022
    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2022

    New changeset 6f9ca53 by Yury Selivanov in branch '3.10':
    bpo-46347: Fix PyEval_EvalCodeEx to correctly cleanup in error paths (bpo-30553)
    6f9ca53

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2022

    New changeset be578e0 by Yury Selivanov in branch 'main':
    bpo-46347: Yet another fix in the erorr path of PyEval_EvalCodeEx (bpo-30554)
    be578e0

    @gpshead
    Copy link
    Member

    gpshead commented Jan 12, 2022

    when was the regression introduced?

    I wouldn't necessarily rush an urgent release out unless this was just introduced in the 3.10.1 patch release. Could it wait for 3.10.2 already scheduled for four weeks from now?

    @elprans
    Copy link
    Mannequin

    elprans mannequin commented Jan 12, 2022

    The leak was introduced in 3.10a5 (0332e56)

    @pablogsal
    Copy link
    Member

    Could it wait for 3.10.2 already scheduled for four weeks from now?

    I don't feel comfortable leaving a ton of Cython functions leaking memory constantly on many function calls. According to MagicStack/asyncpg#874 asyncpg can leak quite a lot of memory depending on your usage pattern.

    Even if this is the first time we heard about this problem, I think this is important enough to justify a extra release.

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

    No branches or pull requests
    6 participants
    @gpshead @vstinner @1st1 @pablogsal @larryhastings @miss-islington