Document PyModule_AddObject's behavior on error

[berkerpeksag]

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]

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.

[serhiy-storchaka]

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]

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]

Added comments on Rietveld.

[serhiy-storchaka]

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]

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

[berkerpeksag]

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

[serhiy-storchaka]

issue26868_v2.diff LGTM.

[brandtbucher]

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]

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]

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

[miss-islington]

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

[hauntsaninja]

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