Skip to content

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

Merged
serhiy-storchaka merged 1 commit intopython:2.7from
zasdfgbnm:patch-1
Feb 28, 2019
Merged

[2.7] bpo-36126: Fix ref count leakage in structseq_repr#12035
serhiy-storchaka merged 1 commit intopython:2.7from
zasdfgbnm:patch-1

Conversation

@zasdfgbnm
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

@eamanu The master branch does not have this problem.

@zasdfgbnm
Copy link
Copy Markdown
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
Copy Markdown
Author

Do I need to add news?

@serhiy-storchaka
Copy link
Copy Markdown
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
Copy Markdown
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