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

PyObject_REPR macro causes refcount leak #66643

Closed
ChrisColbert mannequin opened this issue Sep 21, 2014 · 14 comments
Closed

PyObject_REPR macro causes refcount leak #66643

ChrisColbert mannequin opened this issue Sep 21, 2014 · 14 comments
Assignees
Labels
extension-modules C modules in the Modules dir interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@ChrisColbert
Copy link
Mannequin

ChrisColbert mannequin commented Sep 21, 2014

BPO 22453
Nosy @pitrou, @vstinner, @serhiy-storchaka
Files
  • PyObject_REPR.patch
  • PyObject_REPR_2.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2014-11-18.22:19:38.827>
    created_at = <Date 2014-09-21.16:26:08.398>
    labels = ['extension-modules', 'interpreter-core', 'performance']
    title = 'PyObject_REPR macro causes refcount leak'
    updated_at = <Date 2014-11-18.22:19:38.826>
    user = 'https://bugs.python.org/ChrisColbert'

    bugs.python.org fields:

    activity = <Date 2014-11-18.22:19:38.826>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2014-11-18.22:19:38.827>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2014-09-21.16:26:08.398>
    creator = 'Chris.Colbert'
    dependencies = []
    files = ['37220', '37221']
    hgrepos = []
    issue_num = 22453
    keywords = ['patch']
    message_count = 14.0
    messages = ['227219', '231311', '231313', '231314', '231315', '231316', '231319', '231328', '231329', '231335', '231344', '231346', '231348', '231349']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'vstinner', 'python-dev', 'Chris.Colbert', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue22453'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @ChrisColbert
    Copy link
    Mannequin Author

    ChrisColbert mannequin commented Sep 21, 2014

    This is how the macro is defined in object.h:

    2.7
    /* Helper for passing objects to printf and the like */
    #define PyObject_REPR(obj) PyString_AS_STRING(PyObject_Repr(obj))
    
    3.4
    /* Helper for passing objects to printf and the like */
    #define PyObject_REPR(obj) _PyUnicode_AsString(PyObject_Repr(obj))

    PyObject_Repr returns a new reference, which is not released by the macro.

    This macro only seems to be used internally for error reporting in compile.c, so it's unlikely to be causing any pressing issues for the interpreter, but it may be biting some extension modules.

    @ChrisColbert ChrisColbert mannequin added type-bug An unexpected behavior, bug, or error extension-modules C modules in the Modules dir interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) labels Sep 21, 2014
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Nov 18, 2014

    Thank you for your report Chris.

    PyObject_REPR() is used only in Python/compile.c just before calling Py_FatalError(). So refcount leaks doesn't matter in these cases.

    PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is not defined if Py_LIMITED_API is defined. So it is unlikely that third-party code uses it. We can just remove this macro in 3.5.

    There are other bugs in formatting fatal error messages where PyObject_REPR() is used. PyBytes_AS_STRING() is called for unicode objects. Definitely this branch of the code is rarely executed.

    Here is a patch which expands PyObject_REPR() in Python/compile.c and replaces PyBytes_AS_STRING() with PyUnicode_AsUTF8(). In 3.5 we also should remove the PyObject_REPR macro definition.

    @serhiy-storchaka serhiy-storchaka self-assigned this Nov 18, 2014
    @serhiy-storchaka serhiy-storchaka added performance Performance or resource usage and removed type-bug An unexpected behavior, bug, or error labels Nov 18, 2014
    @vstinner
    Copy link
    Member

    vstinner commented Nov 18, 2014

    PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is not defined if Py_LIMITED_API is defined.

    PyObject_REPR() is still defined if Py_LIMITED_API is defined, I just checked.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Nov 18, 2014

    PyObject_REPR() is still defined if Py_LIMITED_API is defined, I just
    checked.

    But it is expanded to undefined name. So it is not usable in any case.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 18, 2014

    PyObject_REPR.patch: the first part looks good to me. For the second part, you can use PySys_FormatStderr() which is more complex but more correct: it formats the string as Unicode and then encode it to stderr encoding. PyUnicode_FromFormatV() is probably safer to handle errors.

    You may use PySys_FormatStderr() in the two functions to write context, and then call Py_FatalError with a simple message. The exact formatting doesn't matter much, these cases must never occur :-) An assertion may be enough :-p

    @vstinner
    Copy link
    Member

    vstinner commented Nov 18, 2014

    Serhiy Storchaka wrote:

    But it is expanded to undefined name. So it is not usable in any case.

    Ah correct, I didn't notice _PyUnicode_AsString in the expanded result
    (I checked with gcc -E).

    When Py_LIMITED_API is set, PyObject_REPR(o) is expanded to
    _PyUnicode_AsString(PyObject_Repr(o)).

    @pitrou
    Copy link
    Member

    pitrou commented Nov 18, 2014

    PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is
    not defined if Py_LIMITED_API is defined. So it is unlikely that
    third-party code uses it

    Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Nov 18, 2014

    For the second part, you can use PySys_FormatStderr() which is more complex but more correct

    PySys_FormatStderr() is more heavy function, it depends on working sys.stderr and codecs. Taking into account the lack of tests we should be careful with such changes. So I prefer to keep fprintf.

    Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().

    So we should just add a warning? This macro is not documented anywhere.

    -/* Helper for passing objects to printf and the like */
    -#define PyObject_REPR(obj) _PyUnicode_AsString(PyObject_Repr(obj))
    +#ifndef Py_LIMITED_API
    +/* Helper for passing objects to printf and the like.
    +   Leaks refcounts.  Don't use it!
    +*/
    +#define PyObject_REPR(obj) PyUnicode_AsUTF8(PyObject_Repr(obj))
    +#endif

    @pitrou
    Copy link
    Member

    pitrou commented Nov 18, 2014

    Le 18/11/2014 17:39, Serhiy Storchaka a écrit :

    > Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().

    So we should just add a warning? This macro is not documented anywhere.

    Well we can still remove it, and add a porting note in whatsnew for 3.5.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Nov 18, 2014

    Here is updated patch.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 18, 2014

    PyObject_REPR_2.patch looks good to me, but it should only be applied
    to Python 3.5.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 18, 2014

    New changeset e339d75a21d5 by Serhiy Storchaka in branch 'default':
    Issue bpo-22453: Removed non-documented macro PyObject_REPR().
    https://hg.python.org/cpython/rev/e339d75a21d5

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 18, 2014

    New changeset 342a619cdafb by Serhiy Storchaka in branch '3.4':
    Issue bpo-22453: Warn against the use of leaking macro PyObject_REPR().
    https://hg.python.org/cpython/rev/342a619cdafb

    New changeset 6e26b5291c41 by Serhiy Storchaka in branch '2.7':
    Issue bpo-22453: Fexed reference leaks when format error messages in ceval.c.
    https://hg.python.org/cpython/rev/6e26b5291c41

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Nov 18, 2014

    Thank you for your reviews Victor and Antoine.

    @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
    extension-modules C modules in the Modules dir interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants