Brett Cannon added the comment:
On Thu, Feb 14, 2013 at 11:07 AM, Dave Malcolm <rep...@bugs.python.org>wrote:
>
> New submission from Dave Malcolm:
>
> When running my refcount static analyzer [1] on a large corpus of
> real-world C extension code for Python, I ran into the following construct
> in various places:
>
> Py_XDECREF(PyObject_CallObject(callable, args));
>
Eww.
>
> This is bogus code: Py_XDECREF expands its argument multiple times, so
> after this goes through the C preprocessor the callable is actually called
> invoked up to 4 times, leaking refs to 3 of the results - assuming that
> none of the calls fail, and a non pydebug build of CPython (which would
> expand the argument more times).
>
> The raw preprocessor output for an optimized Python 2.7 looks like this:
> do { if ((PyObject_CallObject(callable, args)) == ((void *)0)) ; else
> do { if ( --((PyObject*)(PyObject_CallObject(callable, args)))->ob_refcnt
> != 0) ; else ( (*(((PyObject*)((PyObject *)(PyObject_CallObject(callable,
> args))))->ob_type)->tp_dealloc)((PyObject *)((PyObject
> *)(PyObject_CallObject(callable, args))))); } while (0); } while (0);
>
> Cleaned up (and removing some of the casts for clarity) it looks like this:
> do {
> if ((PyObject_CallObject(callable, args)) == ((void *)0))
> ;
> else
> do {
> if (--(PyObject_CallObject(callable, args)->ob_refcnt) != 0)
> ;
> else
> (*(PyObject_CallObject(callable, args)->ob_type)->tp_dealloc)
> PyObject_CallObject(callable, args);
> } while (0);
> } while (0);
> which is clearly not what the extension author was expecting.
>
> Should we:
> (A) update the macro so that it expands its argument only once, or
>
How expensive is that going to be? Since there is a 'do' statement using a
temp variable would be easy to introduce, but is a compiler going to be
smart enough to inline it all so it doesn't have to waste an allocation,
etc.?
> (B) warn people not to write code like the above?
>
Only if (A) is too costly. I would modify a checkout for ALL refcount
macros and run the benchmark suite to see if there is a noticeable
difference. If there isn't then I say change all the macros (can't make one
safe and the rest not as that is just asking for inconsistent usage and
trouble).
-Brett
>
> Similar considerations apply to the other macros, I guess, but I've seen
> the above used "in the wild", I haven't yet seen it for the other macros).
>
> Seen in the wild in:
> python-alsa-1.0.25-1.fc17:
> pyalsa/alsamixer.c:179
> 00174 | for (i = 0; i < count; i++) {
> 00175 | t = PyTuple_New(2);
> 00176 | if (t) {
> 00177 | PyTuple_SET_ITEM(t, 0,
> PyInt_FromLong(pfd[i].fd));
> 00178 | PyTuple_SET_ITEM(t, 1,
> PyInt_FromLong(pfd[i].events));
> 00179>| Py_XDECREF(PyObject_CallObject(reg, t));
> 00180 | Py_DECREF(t);
> 00181 | }
> 00182 | }
> 00183 |
> 00184 | Py_XDECREF(reg);
>
> pyalsa/alsahcontrol.c:190
> 00185 | for (i = 0; i < count; i++) {
> 00186 | t = PyTuple_New(2);
> 00187 | if (t) {
> 00188 | PyTuple_SET_ITEM(t, 0,
> PyInt_FromLong(pfd[i].fd));
> 00189 | PyTuple_SET_ITEM(t, 1,
> PyInt_FromLong(pfd[i].events));
> 00190>| Py_XDECREF(PyObject_CallObject(reg, t));
> 00191 | Py_DECREF(t);
> 00192 | }
> 00193 | }
> 00194 |
> 00195 | Py_XDECREF(reg);
>
> pyalsa/alsaseq.c:3277
> 03272 | for (i = 0; i < count; i++) {
> 03273 | t = PyTuple_New(2);
> 03274 | if (t) {
> 03275 | PyTuple_SET_ITEM(t, 0, PyInt_FromLong(pfd[i].fd));
> 03276 | PyTuple_SET_ITEM(t, 1,
> PyInt_FromLong(pfd[i].events));
> 03277>| Py_XDECREF(PyObject_CallObject(reg, t));
> 03278 | Py_DECREF(t);
> 03279 | }
> 03280 | }
> 03281 |
> 03282 | Py_XDECREF(reg);
>
> python-4Suite-XML-1.0.2-14.fc17:
> Ft/Xml/src/domlette/refcounts.c:80
> 00075 | /* refcount = this */
> 00076 | expected = 1;
> 00077 | }
> 00078 | else {
> 00079 | sprintf(buf, "Unexpected object type '%.200s'"
> ,node->ob_type->tp_name);
> 00080>| Py_XDECREF(PyObject_CallMethod(tester, "error", "s", buf));
> 00081 | return 0;
> 00082 | }
> 00083 |
> 00084 | sprintf(buf, "%.200s refcounts", node->ob_type->tp_name);
> 00085 | return do_test(tester, buf, expected, node->ob_refcnt);
>
>
> [Note to self: I actually saw this because it uncovered a traceback in
> cpychecker, which I fixed as:
>
> http://git.fedorahosted.org/cgit/gcc-python-plugin.git/commit/?id=99fa820487e380b74c2eda1d97facdf2ee6d2f3a]
>
>
> [1] https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html
>
> ----------
> components: Interpreter Core
> messages: 182107
> nosy: dmalcolm
> priority: normal
> severity: normal
> status: open
> title: Py_XDECREF() expands its argument multiple times
> versions: Python 2.7, Python 3.1, Python 3.2, Python 3.3, Python 3.4,
> Python 3.5
>
> _______________________________________
> Python tracker <rep...@bugs.python.org>
> <http://bugs.python.org/issue17206>
> _______________________________________
> _______________________________________________
> New-bugs-announce mailing list
> new-bugs-annou...@python.org
> http://mail.python.org/mailman/listinfo/new-bugs-announce
>
----------
nosy: +brett.cannon
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue17206>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
