classification
Title: Document PyModule_AddObject's behavior on error
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: 26871 Superseder:
Assigned To: Nosy List: Ma Lin, berker.peksag, brandtbucher, matrixise, miss-islington, nirs, serhiy.storchaka
Priority: low Keywords: patch

Created on 2016-04-27 05:36 by berker.peksag, last changed 2019-09-12 12:29 by miss-islington.

Files
File name Uploaded Description Edit
csv.diff berker.peksag, 2016-04-27 05:36 review
addobject_doc.diff berker.peksag, 2016-04-27 09:52 review
issue26868_v2.diff berker.peksag, 2016-06-10 05:37 review
Pull Requests
URL Status Linked Edit
PR 15725 merged brandtbucher, 2019-09-07 15:52
PR 16037 merged miss-islington, 2019-09-12 12:11
PR 16038 merged miss-islington, 2019-09-12 12:11
Messages (12)
msg264350 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-27 05:36
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.
msg264358 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-27 07:19
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 .
msg264367 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-27 09:52
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.
msg264375 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-27 12:05
Added comments on Rietveld.
msg264388 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-27 18:09
As for Modules/_csv.c, I think it is better to change the behavior of PyModule_AddObject() (issue26871). This will fix similar issues in all modules.
msg268088 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-10 05:37
Thanks for the review Serhiy. Here is an updated patch.
msg276977 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-19 13:29
Serhiy, do you have further comments about issue26868_v2.diff?
msg330940 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-03 11:53
issue26868_v2.diff LGTM.
msg351260 - (view) Author: Brandt Bucher (brandtbucher) * (Python triager) Date: 2019-09-06 15:57
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.
msg352135 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-09-12 12:11
New changeset 224b8aaa7e8f67f748e8b7b6a4a77a25f6554651 by Stéphane Wirtel (Brandt Bucher) in branch 'master':
bpo-26868: Fix example usage of PyModule_AddObject. (#15725)
https://github.com/python/cpython/commit/224b8aaa7e8f67f748e8b7b6a4a77a25f6554651
msg352140 - (view) Author: miss-islington (miss-islington) Date: 2019-09-12 12:26
New changeset 535863e3f599a6ad829204d83f144c91e44de443 by Miss Islington (bot) in branch '3.8':
bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725)
https://github.com/python/cpython/commit/535863e3f599a6ad829204d83f144c91e44de443
msg352141 - (view) Author: miss-islington (miss-islington) Date: 2019-09-12 12:29
New changeset c8d1338441114fbc504625bc66607e7996018a5d by Miss Islington (bot) in branch '3.7':
bpo-26868: Fix example usage of PyModule_AddObject. (GH-15725)
https://github.com/python/cpython/commit/c8d1338441114fbc504625bc66607e7996018a5d
History
Date User Action Args
2019-09-12 12:29:12miss-islingtonsetmessages: + msg352141
2019-09-12 12:26:49miss-islingtonsetnosy: + miss-islington
messages: + msg352140
2019-09-12 12:11:39miss-islingtonsetpull_requests: + pull_request15660
2019-09-12 12:11:32miss-islingtonsetpull_requests: + pull_request15659
2019-09-12 12:11:23matrixisesetnosy: + matrixise
messages: + msg352135
2019-09-07 23:54:25Ma Linsetnosy: + Ma Lin
2019-09-07 15:52:11brandtbuchersetpull_requests: + pull_request15380
2019-09-06 15:57:27brandtbuchersetnosy: + brandtbucher
messages: + msg351260
2019-04-28 22:41:04nirssetnosy: + nirs
2018-12-03 11:53:26serhiy.storchakasetversions: + Python 3.8, - Python 3.5
2018-12-03 11:53:16serhiy.storchakasetmessages: + msg330940
2016-09-19 13:29:00berker.peksagsetmessages: + msg276977
versions: + Python 3.7
2016-06-10 05:37:40berker.peksagsetfiles: + issue26868_v2.diff

messages: + msg268088
2016-04-28 15:31:14berker.peksagsettitle: Incorrect check for return value of PyModule_AddObject in _csv.c -> Document PyModule_AddObject's behavior on error
2016-04-27 18:09:03serhiy.storchakasetdependencies: + Change weird behavior of PyModule_AddObject()
messages: + msg264388
2016-04-27 12:05:49serhiy.storchakasetmessages: + msg264375
2016-04-27 09:52:43berker.peksagsetfiles: + addobject_doc.diff

messages: + msg264367
2016-04-27 07:19:34serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg264358
2016-04-27 05:36:44berker.peksagcreate