Skip to content

[2.7] bpo-36126: Fix ref count leakage in structseq_repr #12035

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

Merged
merged 1 commit into from
Feb 28, 2019
Merged

[2.7] bpo-36126: Fix ref count leakage in structseq_repr #12035

merged 1 commit into from
Feb 28, 2019

Conversation

zasdfgbnm
Copy link

@zasdfgbnm zasdfgbnm commented Feb 25, 2019

Not sure if I need to open an issue for this one line fix.

This fixes the ref count leakage of structseq_repr on Python 2.7. To reproduce the bug, create a new project:

setup.py

# coding=utf8
# this example is modified from https://gist.github.com/saghul/2473614

from distutils.core import setup, Extension

setup(name             = "foo",
      version          = "0.0.1",
      author           = "Saúl Ibarra Corretgé",
      author_email     = "saghul@gmail.com",
      description      = "PyStructSequence example",
      ext_modules      = [Extension('foo',
                                sources = ['test_pystructsequence.c'],
                         )]
     )

test_pystructsequence.c

#include "Python.h"
#include "structmember.h"
#include "structseq.h"

static PyTypeObject FooResultType = {0, 0, 0, 0, 0, 0};

static PyStructSequence_Field foo_result_fields[] = {
    {"field_1", "This is field 1"},
    {"field_2", "This is field 2"},
    {"field_3", "This is field 3"},
    {NULL}
};

static PyStructSequence_Desc foo_result_desc = {
    "foo_result",
    NULL,
    foo_result_fields,
    3
};

#if PY_MAJOR_VERSION >= 3
static struct PyModuleDef moduledef = {
        PyModuleDef_HEAD_INIT,
        "foo",
        NULL,
        0,
        NULL,
        NULL,
        NULL,
        NULL,
        NULL
};

PyMODINIT_FUNC
PyInit_foo(void)
#else
initfoo(void)
#endif
{
#if PY_MAJOR_VERSION >= 3
    PyObject *foo = PyModule_Create(&moduledef);
#else
    PyObject *foo = Py_InitModule("foo", NULL);
#endif

    if (FooResultType.tp_name == 0)
        PyStructSequence_InitType(&FooResultType, &foo_result_desc);
    Py_INCREF((PyObject *) &FooResultType);
    PyModule_AddObject(foo, "foo_result", (PyObject *) &FooResultType);

    // create a new object
    PyObject* obj = PyStructSequence_New(&FooResultType);
    Py_INCREF(obj);
    PyStructSequence_SET_ITEM(obj, 0, Py_None);
    PyStructSequence_SET_ITEM(obj, 1, Py_None);
    PyStructSequence_SET_ITEM(obj, 2, Py_None);
    PyModule_AddObject(foo, "obj", obj);

    FooResultType.tp_members[1].name = NULL;

#if PY_MAJOR_VERSION >= 3
    return foo;
#endif
}

And run the following file and look at the memory usage:

import foo

while True:
    try:
        repr(foo.obj)
    except SystemError:
        pass

https://bugs.python.org/issue36126

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@zasdfgbnm zasdfgbnm changed the title Fix ref count leakage in structseq_repr [2.7] Fix ref count leakage in structseq_repr Feb 25, 2019
@zasdfgbnm
Copy link
Author

Just signed CLA, might take time for the status to be updated?

Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, Can you add a testcase?

hmm IMO you should open a bpo, this way you will have more reviewers

@eamanu
Copy link
Contributor

eamanu commented Feb 26, 2019

Try to made the PR on master branch.

@zasdfgbnm zasdfgbnm changed the title [2.7] Fix ref count leakage in structseq_repr [2.7] bpo-36126: Fix ref count leakage in structseq_repr Feb 26, 2019
@zasdfgbnm
Copy link
Author

@eamanu The master branch does not have this problem.

@zasdfgbnm
Copy link
Author

Also @eamanu, is there a way to test ref count for local variables is correctly handled? These variables are not visible outside the function. Also, there are so many edge cases, and I didn't see any similar test for other edge cases.

@zasdfgbnm
Copy link
Author

Do I need to add news?

@serhiy-storchaka
Copy link
Member

I think this is not required. You can get the leak only in very special situation. SystemError itself is a sign of more severe issues.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error skip news labels Feb 28, 2019
@serhiy-storchaka serhiy-storchaka merged commit 69b4a17 into python:2.7 Feb 28, 2019
@serhiy-storchaka
Copy link
Member

Thank you for your contribution @zasdfgbnm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants