tuple creation in C extensions

[Bernhard Herzog]

Alex <cut_me_out at hotmail.com> writes:

> > Do keep digging. Ref-counting is important.
> 
> Ref-counting errors can be such a pain in the neck to track down that I
> have taken to placing assertions at the end of routines to make sure
> the ref-counts are what they ought to be.

That's usually very hard to get right. It's usually very hard to guess
what the reference count of a given object should be.

> This is the sort of thing I've been doing:
> 
> int dict_add_pattern (PyObject *dict, char *string, int length, long pos[DEPTH]) {
>   PyObject *pattern, *coords, *value, *tuple;
>   int return_code, pattern_existed;
> 
>   assert (length <= DEPTH);
> 
>   coords = dict_set_coords (length - 1, pos);
>   if (!coords) {goto fail;}
>   
>   pattern = Py_BuildValue ("s#N", string, length, coords);
>   if (!pattern) {Py_DECREF (coords); goto fail;}
 
>   value = PyDict_GetItem(dict, pattern);
>   if (!PyDict_GetItem(dict, pattern)) {
>     pattern_existed = 0;
>     value = PyInt_FromLong( 1 );
>     return_code = PyDict_SetItem(dict, pattern, value);

You have to DECREF value here.

>   } else {
>     pattern_existed = 1;
>     assert (PyInt_Check( value ));
>     value = PyNumber_Add( value, PyInt_FromLong( 1 ));

Here's a memory leak for the old value and the 'new' intobject returned
by PyInt_FromLong. 

>     if (value == NULL) {goto fail;}

If value is indeed NULL, you leak the reference to pattern

>     return_code = PyDict_SetItem(dict, pattern, value);

You have to DECREF value here.

>   }
>   /* If a tuple equal to pattern was already in dict, then pattern will not be
>      referred to by dict, and should have a refcnt of 0.  If there was no such
>      tuple, then PyDict_SetItem increased the refcnt of dict by one.  In
>      either case, the refcnt has to be decremented, now.
>      */
>   Py_DECREF (pattern);

The DECREF of pattern may delete pattern so pattern should not be
dereferenced after this point. Since pattern has the only reference to
coords, coords may also have been deleted.

>       
>   if (return_code == -1) {
>     goto fail;
>   } else {
>     if (DEBUG_REFCOUNTS) {
>       printf ("coords: %i\npattern: %i\nvalue: %i\n",
> 	      coords->ob_refcnt, pattern->ob_refcnt, value->ob_refcnt);
>     }
>     if (DEBUG_LIST_PATTERNS) {
>       PyObject_Print (dict, stdout, 0); printf ("\n");
>     }
>     goto pass;
>   }
> 
> fail:
>   return 0;
> pass:
>   assert (pattern->ob_refcnt == (!pattern_existed));
>   assert ((coords->ob_refcnt == (!pattern_existed)) | (length == 1));

You dereference potentially free()'ed pointers here and in the debug
messages above. To make things worse, if DEBUG_LIST_PATTERNS is true,
PyObject_Print might execute pretty arbitrary Python code which might
result in the memory being reused again.

>   return 1;
> }

-- 
Bernhard Herzog   | Sketch, a drawing program for Unix
herzog at online.de  | http://sketch.sourceforge.net/