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.

Sign up for GitHub

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

Closed
berkerpeksag opened this issue Apr 27, 2016 · 13 comments
Closed

Document PyModule_AddObject's behavior on error #71055

berkerpeksag opened this issue Apr 27, 2016 · 13 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@berkerpeksag
Copy link
Member

BPO 26868
Nosy @nirs, @berkerpeksag, @serhiy-storchaka, @matrixise, @animalize, @miss-islington, @brandtbucher
PRs
  • bpo-26868: Fix example usage of PyModule_AddObject. #15725
  • [3.8] bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725) #16037
  • [3.7] bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725) #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

    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 C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Apr 27, 2016
    @serhiy-storchaka
    Copy link
    Member

    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

    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

    Added comments on Rietveld.

    @serhiy-storchaka
    Copy link
    Member

    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

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

    @berkerpeksag
    Copy link
    Member Author

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

    @berkerpeksag berkerpeksag added the 3.7 (EOL) end of life label Sep 19, 2016
    @serhiy-storchaka
    Copy link
    Member

    issue26868_v2.diff LGTM.

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Dec 3, 2018
    @brandtbucher
    Copy link
    Member

    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

    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

    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

    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
    @hauntsaninja
    Copy link
    Contributor

    This has been documented, looks like #83004 is to track fixing the various call sites

    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 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    6 participants
    @berkerpeksag @matrixise @serhiy-storchaka @hauntsaninja @miss-islington @brandtbucher