classification
Title: PyObject_REPR macro causes refcount leak
Type: resource usage Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Chris.Colbert, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2014-09-21 16:26 by Chris.Colbert, last changed 2014-11-18 22:19 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
PyObject_REPR.patch serhiy.storchaka, 2014-11-18 10:40 review
PyObject_REPR_2.patch serhiy.storchaka, 2014-11-18 18:06 review
Messages (14)
msg227219 - (view) Author: Chris Colbert (Chris.Colbert) Date: 2014-09-21 16:26
This is how the macro is defined in object.h:

2.7
/* Helper for passing objects to printf and the like */
#define PyObject_REPR(obj) PyString_AS_STRING(PyObject_Repr(obj))

3.4
/* Helper for passing objects to printf and the like */
#define PyObject_REPR(obj) _PyUnicode_AsString(PyObject_Repr(obj))


PyObject_Repr returns a new reference, which is not released by the macro. 

This macro only seems to be used internally for error reporting in compile.c, so it's unlikely to be causing any pressing issues for the interpreter, but it may be biting some extension modules.
msg231311 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-18 10:40
Thank you for your report Chris.

PyObject_REPR() is used only in Python/compile.c just before calling Py_FatalError(). So refcount leaks doesn't matter in these cases.

PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is not defined if Py_LIMITED_API is defined. So it is unlikely that third-party code uses it. We can just remove this macro in 3.5.

There are other bugs in formatting fatal error messages where PyObject_REPR() is used. PyBytes_AS_STRING() is called for unicode objects. Definitely this branch of the code is rarely executed.

Here is a patch which expands PyObject_REPR() in Python/compile.c and replaces PyBytes_AS_STRING() with PyUnicode_AsUTF8(). In 3.5 we also should remove the PyObject_REPR macro definition.
msg231313 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-18 12:10
> PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is not defined if Py_LIMITED_API is defined.

PyObject_REPR() is still defined if Py_LIMITED_API is defined, I just checked.
msg231314 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-18 12:16
> PyObject_REPR() is still defined if Py_LIMITED_API is defined, I just
> checked.

But it is expanded to undefined name. So it is not usable in any case.
msg231315 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-18 12:19
PyObject_REPR.patch: the first part looks good to me. For the second part, you can use PySys_FormatStderr() which is more complex but more correct: it formats the string as Unicode and then encode it to stderr encoding. PyUnicode_FromFormatV() is probably safer to handle errors.

You may use PySys_FormatStderr() in the two functions to write context, and then call Py_FatalError with a simple message. The exact formatting doesn't matter much, these cases must never occur :-) An assertion may be enough :-p
msg231316 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-18 12:22
Serhiy Storchaka wrote:
> But it is expanded to undefined name. So it is not usable in any case.

Ah correct, I didn't notice _PyUnicode_AsString in the expanded result
(I checked with gcc -E).

When Py_LIMITED_API is set, PyObject_REPR(o) is expanded to
_PyUnicode_AsString(PyObject_Repr(o)).
msg231319 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-18 13:35
> PyObject_REPR() is expanded to deprecated _PyUnicode_AsString which is 
> not defined if Py_LIMITED_API is defined. So it is unlikely that 
> third-party code uses it

Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().
msg231328 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-18 16:39
> For the second part, you can use PySys_FormatStderr() which is more complex but more correct

PySys_FormatStderr() is more heavy function, it depends on working sys.stderr and codecs. Taking into account the lack of tests we should be careful with such changes. So I prefer to keep fprintf.

> Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().

So we should just add a warning? This macro is not documented anywhere.

-/* Helper for passing objects to printf and the like */
-#define PyObject_REPR(obj) _PyUnicode_AsString(PyObject_Repr(obj))
+#ifndef Py_LIMITED_API
+/* Helper for passing objects to printf and the like.
+   Leaks refcounts.  Don't use it!
+*/
+#define PyObject_REPR(obj) PyUnicode_AsUTF8(PyObject_Repr(obj))
+#endif
msg231329 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-18 16:55
Le 18/11/2014 17:39, Serhiy Storchaka a écrit :
> 
>> Py_LIMITED_API is the "stable ABI". Most third-party code doesn't use it, so it may still use PyObject_REPR().
> 
> So we should just add a warning? This macro is not documented anywhere.

Well we can still remove it, and add a porting note in whatsnew for 3.5.
msg231335 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-18 18:06
Here is updated patch.
msg231344 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-11-18 20:51
PyObject_REPR_2.patch looks good to me, but it should only be applied
to Python 3.5.
msg231346 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-18 21:36
New changeset e339d75a21d5 by Serhiy Storchaka in branch 'default':
Issue #22453: Removed non-documented macro PyObject_REPR().
https://hg.python.org/cpython/rev/e339d75a21d5
msg231348 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-18 22:17
New changeset 342a619cdafb by Serhiy Storchaka in branch '3.4':
Issue #22453: Warn against the use of leaking macro PyObject_REPR().
https://hg.python.org/cpython/rev/342a619cdafb

New changeset 6e26b5291c41 by Serhiy Storchaka in branch '2.7':
Issue #22453: Fexed reference leaks when format error messages in ceval.c.
https://hg.python.org/cpython/rev/6e26b5291c41
msg231349 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-18 22:19
Thank you for your reviews Victor and Antoine.
History
Date User Action Args
2014-11-18 22:19:38serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg231349

stage: patch review -> resolved
2014-11-18 22:17:27python-devsetmessages: + msg231348
2014-11-18 21:36:48python-devsetnosy: + python-dev
messages: + msg231346
2014-11-18 20:51:01vstinnersetmessages: + msg231344
2014-11-18 18:06:46serhiy.storchakasetfiles: + PyObject_REPR_2.patch

messages: + msg231335
2014-11-18 16:55:36pitrousetmessages: + msg231329
2014-11-18 16:39:07serhiy.storchakasetmessages: + msg231328
2014-11-18 13:35:06pitrousetnosy: + pitrou
messages: + msg231319
2014-11-18 12:22:44vstinnersetmessages: + msg231316
2014-11-18 12:19:25vstinnersetmessages: + msg231315
2014-11-18 12:16:39serhiy.storchakasetmessages: + msg231314
2014-11-18 12:10:46vstinnersetnosy: + vstinner
messages: + msg231313
2014-11-18 10:40:50serhiy.storchakasetfiles: + PyObject_REPR.patch

type: behavior -> resource usage
assignee: serhiy.storchaka
versions: - Python 3.1, Python 3.2, Python 3.3
keywords: + patch
nosy: + serhiy.storchaka

messages: + msg231311
stage: patch review
2014-09-21 16:26:08Chris.Colbertcreate