Repository navigation
Document PyModule_AddObject's behavior on error #71055
Description
| BPO | 26868 |
|---|---|
| Nosy | @nirs, @berkerpeksag, @serhiy-storchaka, @matrixise, @animalize, @miss-islington, @brandtbucher |
| PRs | |
| Dependencies | |
| Files |
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']
Activity
berkerpeksag commentedon Apr 27, 2016
This is probably harmless, but Modules/_csv.c has the following code:
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:
The same pattern can be found in a few more modules.
serhiy-storchaka commentedon Apr 27, 2016
Testing returned value of PyModule_AddObject() is correct. This is a matter of style what to use:
if (...),if (... == -1)orif (... < 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 commentedon 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 commentedon Apr 27, 2016
Added comments on Rietveld.
serhiy-storchaka commentedon 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.
[-]Incorrect check for return value of PyModule_AddObject in _csv.c[/-][+]Document PyModule_AddObject's behavior on error[/+]berkerpeksag commentedon Jun 10, 2016
Thanks for the review Serhiy. Here is an updated patch.
berkerpeksag commentedon Sep 19, 2016
Serhiy, do you have further comments about issue26868_v2.diff?
serhiy-storchaka commentedon Dec 3, 2018
issue26868_v2.diff LGTM.
brandtbucher commentedon 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 commentedon 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 commentedon 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 commentedon 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
hauntsaninja commentedon Oct 29, 2022
This has been documented, looks like #83004 is to track fixing the various call sites