classification
Title: _ctypes.c: refleak
Type: resource usage Stage: resolved
Components: ctypes Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: meador.inge Nosy List: Suman.Saha, amaury.forgeotdarc, belopolsky, ezio.melotti, georg.brandl, meador.inge, python-dev, terry.reedy
Priority: normal Keywords:

Created on 2011-09-20 14:48 by Suman.Saha, last changed 2011-09-28 02:03 by meador.inge. This issue is now closed.

Files
File name Uploaded Description Edit
python_patch1 Suman.Saha, 2011-09-20 14:48 Patch review
Messages (9)
msg144330 - (view) Author: Suman Saha (Suman.Saha) Date: 2011-09-20 14:48
Something that is allocated using PyTuple_Pack is not freed on one error path.
msg144363 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-09-21 02:38
I'll take this one.

Suman, thanks for finding this.  It will help in the future if you don't
open a ton of bugs with the *exact* same title.  They are harder to 
filter and keep track of that way.
msg144397 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-09-22 02:30
This patch seems reasonable.  Upon looking at the code in more
detail, however, I can't see how the error condition that leaks the
reference can ever by triggered.  In particular, the following code
from the 'PyCArrayType_from_ctype' function:

    if (!PyType_Check(itemtype)) {
        PyErr_SetString(PyExc_TypeError,
                        "Expected a type object");
        return NULL;
    }

'PyCArrayType_from_ctype' is only called from 'CDataType_repeat'.  The
'CDataType_repeat' function is only used to implement the 'sq_repeat' 
slot in 'CDataType_as_sequence'.  'CDataType_as_sequence' is used in all 
of the implemented ctypes (struct, array, union, simple, ...).

Unless 'PyCArrayType_from_ctype' is called through some other means
(it is public), then 'itemtype' should *always* be a type.  Or am 
I missing something obvious?  So, we could add the decref or just remove 
the type check code all together (and make 'PyCArrayType_from_ctype'
private).
msg144472 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-09-23 19:36
My impression is that plugging refleaks (unlike minor speedups) is a bugfix rather than feature addition, so this and the other issues should be marked for 2.7 and 3.2 also. (I am only changing this one.)

Deprecating a public (but obscure) CAPI function is a separate issue that would only affect 3.3 at the earliest (with a PendingDeprecation or Deprecation warning) and would be in addition to plugging the potential leak in the existing code.
msg144493 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-09-24 08:29
I vote for plugging the refleak and keeping the function non-static.
msg144535 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-09-26 01:01
OK, I will just fix the ref leak in 2.7, 3.2, and 3.3.  This is a pretty low-risk fix (as I mentioned before, I can't see how the error condition is even executed).
msg144539 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-09-26 14:31
If the function is public I guess that some external module might use it, and possibly pass a wrong argument that triggers the leak.
msg144540 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-09-26 14:41
> If the function is public I guess that some external module
> might use it

Agreed; That is the only case I could deduce as well, which I hinted at 
in msg144397.  So, I will leave the error check and keep the function
public for now.

I will commit the ref leak fix today.
msg144560 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-09-28 01:57
New changeset 1fb5b0cc6367 by Meador Inge in branch '2.7':
Issue #13013: ctypes: Fix a reference leak in PyCArrayType_from_ctype.
http://hg.python.org/cpython/rev/1fb5b0cc6367

New changeset 58a75eeb5c8e by Meador Inge in branch '3.2':
Issue #13013: ctypes: Fix a reference leak in PyCArrayType_from_ctype.
http://hg.python.org/cpython/rev/58a75eeb5c8e

New changeset 1726fa560112 by Meador Inge in branch 'default':
Issue #13013: ctypes: Fix a reference leak in PyCArrayType_from_ctype.
http://hg.python.org/cpython/rev/1726fa560112
History
Date User Action Args
2011-09-28 02:03:31meador.ingesetstatus: open -> closed
resolution: fixed
stage: test needed -> resolved
2011-09-28 01:57:25python-devsetnosy: + python-dev
messages: + msg144560
2011-09-26 14:41:06meador.ingesetmessages: + msg144540
2011-09-26 14:31:06ezio.melottisetnosy: + ezio.melotti
messages: + msg144539
2011-09-26 01:01:02meador.ingesetmessages: + msg144535
2011-09-24 08:29:39georg.brandlsetnosy: + georg.brandl
messages: + msg144493
2011-09-23 19:36:17terry.reedysetnosy: + terry.reedy

messages: + msg144472
versions: + Python 2.7, Python 3.2
2011-09-22 02:30:46meador.ingesetnosy: + amaury.forgeotdarc, belopolsky
messages: + msg144397
2011-09-21 19:24:09skrahsettitle: Resource is not released before returning from the functiion -> _ctypes.c: refleak
2011-09-21 02:38:22meador.ingesetversions: + Python 3.3
nosy: + meador.inge

messages: + msg144363

assignee: meador.inge
stage: test needed
2011-09-20 18:41:22mark.dickinsonsetcomponents: + ctypes
2011-09-20 14:48:10Suman.Sahacreate