| msg264350 - (view) |
Author: Berker Peksag (berker.peksag) *  |
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) * |
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) * |
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) * |
Date: 2016-04-27 12:05 |
| Added comments on Rietveld. |
|
|
| msg264388 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-27 18:09 |
| As for Modules/_csv.c, I think it is better to change the behavior of PyModule_AddObject() (). This will fix similar issues in all modules. |
|
|
| msg268088 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2016-06-10 05:37 |
| Thanks for the review Serhiy. Here is an updated patch. |
|
|
| msg276977 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2016-09-19 13:29 |
| Serhiy, do you have further comments about issue26868_v2.diff? |
|
|
| msg330940 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-12-03 11:53 |
| issue26868_v2.diff LGTM. |
|
|
| msg351260 - (view) |
Author: Brandt Bucher (brandtbucher) * |
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) * |
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 |
|
|