[New-bugs-announce] [issue17206] Py_XDECREF() expands its argument multiple times

[Dave Malcolm]

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));

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

----------
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 <report at bugs.python.org>
<http://bugs.python.org/issue17206>
_______________________________________