classification
Title: typeobject.c call_method & call_maybe can leak references on 'func'
Type: resource usage Stage: patch review
Components: Interpreter Core Versions: Python 3.6, Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Myron Walker, abarry, martin.panter
Priority: normal Keywords: patch

Created on 2015-11-23 22:44 by Myron Walker, last changed 2015-12-07 23:18 by martin.panter.

Files
File name Uploaded Description Edit
call-leak.patch martin.panter, 2015-11-24 02:02 review
Messages (7)
msg255235 - (view) Author: Myron Walker (Myron Walker) Date: 2015-11-23 22:44
The 'call_method' and 'call_maybe' function in typeobject.c are leaking
a reference on 'func' in cases where 'args == NULL'.  In this case both 
of these functions exit like so:

    if (args == NULL)
        return NULL;

When they need to do:

    if (args == NULL)
    {
        Py_DECREF(func);
        return NULL;
    }
msg255242 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-24 02:02
Thanks, here is a patch with the proposed fix. I also removed an unused macro.

Do you have an easy way to verify this, e.g. by calling a method from Python with invalid arguments? Or is this just something you found by inspection?
msg256068 - (view) Author: Myron Walker (Myron Walker) Date: 2015-12-07 21:01
Found this with code inspection
msg256073 - (view) Author: Myron Walker (Myron Walker) Date: 2015-12-07 21:42
I think this usage of 'call_method' from typeobject.c would cause it to leak.

static PyObject*
slot_sq_slice(PyObject *self, Py_ssize_t i, Py_ssize_t j)
{
	static PyObject *getslice_str;

	if (PyErr_WarnPy3k("in 3.x, __getslice__ has been removed; "
			    "use __getitem__", 1) < 0)
		return NULL;
	return call_method(self, "__getslice__", &getslice_str,
		"nn", i, j);      <<<<<<< Maybe Bad Format Str <<<<<<<
}

Looks like the only place in 'call_method' that could cause args to be NULL is if 'Py_VaBuildValue' which calls 'va_build_value' returns NULL.  'va_build_value' calls 'countformat' on the format string and 'countformat' looks for some escaping in the format string.  If no special format characters like '(' or '{' are not found, then the count might be incremented but when the NULL terminator on the end of the string is hit, the 'countformat' will return -1.

So I believe this method has a bug because it doesn't look to be using the formatter correctly and also it looks like it might cause a leak.
msg256074 - (view) Author: Myron Walker (Myron Walker) Date: 2015-12-07 21:45
I'll file a seperate bug on 'countformat'
msg256079 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2015-12-07 22:00
Could you provide actual code where a reference is leaked? To me, this looks like hypothetical failure rather than something that has a chance of occurring - feel free to prove me wrong, though :) Please also include relevant tests.
msg256087 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-07 23:18
I don’t see any bug with countformat(), but if you can explain in more detail that would be good.

Myron, you are right that the build-value function has to fail to trigger this leak. But from my reading of the code, I think it would probably only happen for an out-of-memory error (e.g. allocating the tuple return value). Both call_method() and call_maybe() are static, and all the call sites look like they give valid format strings that would not fail except for the out-of-memory condition.

Emanuel: if this can only produced by an out-of-memory error, it would be hard to demonstrate the leak. (Maybe use Victor Stinner’s malloc failure tool? I never tried it.) But IMO the code is incorrect as it is. If it is going to be handling an error, it should be cleaning up properly.
History
Date User Action Args
2015-12-07 23:18:55martin.pantersetmessages: + msg256087
2015-12-07 22:00:58abarrysetnosy: + abarry
messages: + msg256079
2015-12-07 21:45:50Myron Walkersetmessages: + msg256074
2015-12-07 21:42:10Myron Walkersetmessages: + msg256073
2015-12-07 21:01:21Myron Walkersetmessages: + msg256068
2015-11-24 02:02:11martin.pantersetfiles: + call-leak.patch

versions: - Python 3.2, Python 3.3
keywords: + patch
nosy: + martin.panter

messages: + msg255242
stage: patch review
2015-11-23 22:52:20Myron Walkersetversions: + Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
2015-11-23 22:44:39Myron Walkercreate