X Tutup
The Wayback Machine - https://web.archive.org/web/20220706131641/https://github.com/python/cpython/issues/71055
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

Document PyModule_AddObject's behavior on error #71055

Open
berkerpeksag opened this issue Apr 27, 2016 · 12 comments
Open

Document PyModule_AddObject's behavior on error #71055

berkerpeksag opened this issue Apr 27, 2016 · 12 comments
Labels
3.7 3.8 extension-modules type-bug

Comments

@berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented Apr 27, 2016

BPO 26868
Nosy @nirs, @berkerpeksag, @serhiy-storchaka, @matrixise, @animalize, @miss-islington, @brandtbucher
PRs
  • #15725
  • #16037
  • #16038
    		•
    Dependencies
  • bpo-26871: Change weird behavior of PyModule_AddObject()
    		•
    Files
  • csv.diff
  • addobject_doc.diff
  • issue26868_v2.diff
    		•

    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 = None
created_at = <Date 2016-04-27.05:36:44.758>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = "Document PyModule_AddObject's behavior on error"
updated_at = <Date 2019-09-12.12:29:12.788>
user = 'https://github.com/berkerpeksag'

    bugs.python.org fields:

    activity = <Date 2019-09-12.12:29:12.788>
actor = 'miss-islington'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2016-04-27.05:36:44.758>
creator = 'berker.peksag'
dependencies = ['26871']
files = ['42623', '42630', '43328']
hgrepos = []
issue_num = 26868
keywords = ['patch']
message_count = 12.0
messages = ['264350', '264358', '264367', '264375', '264388', '268088', '276977', '330940', '351260', '352135', '352140', '352141']
nosy_count = 7.0
nosy_names = ['nirs', 'berker.peksag', 'serhiy.storchaka', 'matrixise', 'malin', 'miss-islington', 'brandtbucher']
pr_nums = ['15725', '16037', '16038']
priority = 'low'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26868'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']
    @berkerpeksag
    Copy link
    Member Author

    @berkerpeksag berkerpeksag commented Apr 27, 2016

    This is probably harmless, but Modules/_csv.c has the following code:

        Py_INCREF(&Dialect_Type);
    if (PyModule_AddObject(module, "Dialect", (PyObject *)&Dialect_Type))
        return NULL;

    However, PyModule_AddObject returns only -1 and 0. It also doesn't decref Dialect_Type if it returns -1 so I guess more correct code should be:

        Py_INCREF(&Dialect_Type);
    if (PyModule_AddObject(module, "Dialect", (PyObject *)&Dialect_Type) == -1) {
        Py_DECREF(&Dialect_Type);
        return NULL;
    }

    The same pattern can be found in a few more modules.

    @berkerpeksag berkerpeksag added extension-modules type-bug labels Apr 27, 2016
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 27, 2016

    Testing returned value of PyModule_AddObject() is correct. This is a matter of style what to use: if (...), if (... == -1) or if (... < 0).

    But the problem with a leak is more general. I have opened a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/157545 .

    @berkerpeksag
    Copy link
    Member Author

    @berkerpeksag berkerpeksag commented Apr 27, 2016

    Thanks for the write-up, Serhiy.

    It looks like "... == -1" is more popular in the codebase (for PyModule_AddObject, "... < 0" is the most popular style).

    Here is a patch to document the current behavior of PyModule_AddObject.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 27, 2016

    Added comments on Rietveld.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 27, 2016

    As for Modules/_csv.c, I think it is better to change the behavior of PyModule_AddObject() (bpo-26871). This will fix similar issues in all modules.

    @berkerpeksag berkerpeksag changed the title Incorrect check for return value of PyModule_AddObject in _csv.c Document PyModule_AddObject's behavior on error Apr 28, 2016
    @berkerpeksag
    Copy link
    Member Author

    @berkerpeksag berkerpeksag commented Jun 10, 2016

    Thanks for the review Serhiy. Here is an updated patch.

    @berkerpeksag
    Copy link
    Member Author

    @berkerpeksag berkerpeksag commented Sep 19, 2016

    Serhiy, do you have further comments about issue26868_v2.diff?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 3, 2018

    issue26868_v2.diff LGTM.

    @brandtbucher
    Copy link
    Member

    @brandtbucher brandtbucher commented Sep 6, 2019

    It looks like the idiom of calling PyModule_AddObject without Py_DECREF'ing on the error condition (or even checking for it at all) has spread quite a bit since this first reported. I'm preparing a PR to fix the other call sites.

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Sep 12, 2019

    New changeset 224b8aa by Stéphane Wirtel (Brandt Bucher) in branch 'master':
    bpo-26868: Fix example usage of PyModule_AddObject. (bpo-15725)
    224b8aa

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Sep 12, 2019

    New changeset 535863e by Miss Islington (bot) in branch '3.8':
    bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725)
    535863e

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Sep 12, 2019

    New changeset c8d1338 by Miss Islington (bot) in branch '3.7':
    bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725)
    c8d1338

    @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.7 3.8 extension-modules type-bug
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @berkerpeksag @matrixise @serhiy-storchaka @miss-islington @brandtbucher
    X Tutup