classification
Title: PyModule_AddIntConstant and PyModule_AddStringConstant can leak
Type: resource usage Stage:
Components: Interpreter Core Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, georg.brandl, gvanrossum, hniksic
Priority: normal Keywords:

Created on 2008-01-10 14:47 by hniksic, last changed 2008-01-19 18:02 by georg.brandl. This issue is now closed.

Files
File name Uploaded Description Edit
addpatch hniksic, 2008-01-14 08:39
Messages (7)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-01-19 18:02
Committed in r60084.
History
Date User Action Args
2008-01-19 18:02:57georg.brandlsetstatus: open -> closed
nosy: + georg.brandl
resolution: fixed
messages: + msg60205
2008-01-14 08:39:39hniksicsetfiles: + addpatch
messages: + msg59892
2008-01-11 15:15:31christian.heimessetmessages: + msg59716
2008-01-11 08:46:39hniksicsetmessages: + msg59698
2008-01-10 17:31:40gvanrossumsetnosy: + gvanrossum
messages: + msg59673
2008-01-10 16:10:07christian.heimessetpriority: normal
nosy: + christian.heimes
messages: + msg59664
versions: + Python 2.6, Python 3.0, - Python 2.5
2008-01-10 14:47:50hniksiccreate