| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-11-18 18:06 |
| Here is updated patch. |
|
|
| msg231344 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
Date: 2014-11-18 22:19 |
| Thank you for your reviews Victor and Antoine. |
|
|