davidmalcolm
commented
11 years ago 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));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 (B) warn people not to write code like the above?
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
brettcannon
benjaminp
a2d4ba29-aed4-49f1-9357-07b008f7d717
mdickinson
amauryfa
vstinner
markshannon
larryhastings
malemburg
1762cc99-3127-4a62-9baf-30c3d0f51ef7
jkloth
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields: ```python assignee = None closed_at =
created_at =
labels = ['interpreter-core', 'type-bug']
title = 'Py_XDECREF() expands its argument multiple times'
updated_at =
user = 'https://github.com/davidmalcolm'
```
bugs.python.org fields:
```python
activity =
actor = 'Mark.Shannon'
assignee = 'none'
closed = True
closed_date =
closer = 'SilentGhost'
components = ['Interpreter Core']
creation =
creator = 'dmalcolm'
dependencies = []
files = ['29439', '29440', '29523', '29524', '29617', '29660', '30625', '30690', '30750']
hgrepos = []
issue_num = 17206
keywords = ['patch']
message_count = 48.0
messages = ['182107', '182111', '182209', '184430', '184431', '184432', '184433', '184488', '184492', '184495', '184833', '184834', '184855', '185396', '185614', '185623', '185625', '185627', '185643', '185689', '185889', '186386', '189362', '189379', '190156', '190623', '190628', '191364', '191731', '191752', '191780', '191785', '191786', '191791', '191792', '191877', '191878', '191879', '191880', '191881', '191883', '191888', '191919', '192167', '192204', '192205', '192206', '192507']
nosy_count = 15.0
nosy_names = ['lemburg', 'brett.cannon', 'jcea', 'amaury.forgeotdarc', 'mark.dickinson', 'vstinner', 'larry', 'benjamin.peterson', 'jkloth', 'Arfrever', 'asvetlov', 'dmalcolm', 'Mark.Shannon', 'python-dev', 'ilblackdragon']
pr_nums = ['30855']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17206'
versions = ['Python 3.4']
```