| msg59662 - (view) |
Author: Hrvoje Nikšić (hniksic) * |
Date: 2008-01-10 14:47 |
| PyModule_AddObject has somewhat strange reference-counting behavior in that it *conditionally* steals a reference. In case of error it doesn't change the reference to the passed object, but in case of success it steals it. This means that, as written, PyModule_AddIntConstant and PyModuleAddStringConstant can leak created objects if PyModule_AddObject fails. As far as I can tell, the correct way to write those functions would be (using PyModule_AddIntConstant as the example): int PyModule_AddIntConstant(PyObject *m, const char *name, long value) { PyObject *o = PyInt_FromLong(value); if (PyModule_AddObject(m, name, o) == 0) return 0; Py_XDECREF(o); return -1; } PyModule_AddObject was obviously intended to enable writing the "simple" code (it even gracefully handles being passed NULL object to add) like the one in PyModule_AddIntConstant, but I don't see a way to enable such usage and avoid both leaks and an interface change. Changing the reference-counting behavior of PyModule_AddObject would be backward-incompatible, but it might be a good idea to consider it for Python 3. If there is agreement that my analysis and the proposed fixes are correct, I will produce a proper patch. |
|
|
| msg59664 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2008-01-10 16:10 |
| Thanks for your analysis! I *think* you are right but I've to study the code more carefully before I can make a decision. We can't target the change for 2.5 though. Guido would accuse my of being insane again. *g* The problem should be discussed at the upcoming bug day. |
|
|
| msg59673 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2008-01-10 17:31 |
| These are meant purely for the convenience of module initialization, and there correct handling of reference counts in the light of failures is of marginal importance. So while these may technically have leaks, you shouldn't care. |
|
|
| msg59698 - (view) |
Author: Hrvoje Nikšić (hniksic) * |
Date: 2008-01-11 08:46 |
| I agree that a leak would very rarely occur in practice, but since there is a straightforward fix, why not apply it? If nothing else, the code in the core should be an example of writing leak-free Python/C code, and a fix will also prevent others from wasting time on this in the future. |
|
|
| msg59716 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-01-11 15:15 |
| I don't mind if you like to pursue the issue. I won't invest any time into it. But if you can come up with a patch we can surely apply it. |
|
|
| msg59892 - (view) |
Author: Hrvoje Nikšić (hniksic) * |
Date: 2008-01-14 08:39 |
| Here is a patch, as per description above. |
|
|
| msg60205 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-01-19 18:02 |
| Committed in r60084. |
|
|