Re: psycopg2 2.0.8 - segmentation fault

On Mon, Apr 20, 2009 at 5:28 AM, Gangadharan S.A. <[email protected]> wrote:
>> I managed to reproduce and fix the issue. This is happening because when
>> dealloc-ing, the "Py_BEGIN_ALLOW_THREADS;" in conn_close is letting other
>> threads launch the garbage collector which in turn ends up running the
>> dealloc a second time.
>>
>> The fix is to untrack the connnection object from GC, before going ahead
>> with dealloc, as said in http://docs.python.org/c-api/gcsupport.html
>
> There was a similar problem with cursor class. Cursor dealloc can internally
> invoke connection dealloc and hence we can get into the same double dealloc
> problem for cursor deallocation. The effects of this double dealloc are not
> immediately visible though, as cursor dealloc seems idempotent for the most
> part. It was leading to gradual memory corruption and eventually a
> segmentation fault.
>
> Have attached the script to reproduce this issue and the one line fix for
> it.
>
> There are other issues I noticed. Unfortunately, I won't be able to spend
> time on these in the near future, so I'm putting them out here for anyone
> else who can fix them. If not, I will try and get to these when time
> permits:
>
> 1. All / most of the classes in psycopg2 code seem to have
> Py_TPFLAGS_HAVE_GC, so they need to a PyObject_GC_UnTrack as their first
> step: http://docs.python.org/c-api/gcsupport.html . But since psycopg2
> classes haven't implemented tp_clear either, the actual double dealloc
> probably won't happen, unless people inherit these classes. (Heap classes
> get the subtype_clear, tp_clear implementation.)

If the type only uses Py_CLEAR() or equivalent to free its resources,
then it isn't necessary to untrack the object (although it probably
won't hurt).

This macro essentially does the following:

    tmp = self->whatever;
    self->whatever = NULL;
    if (tmp != NULL) {
        /* free tmp */
    }

Given that the code is protected by the GIL, there isn't a race in
unsetting the object.


> 2. Classes should implement tp_clear wherever a cycle is possible (such as a
> connection object and its "async_cursor" cursor object). Without this, there
> is a memory leak of cyclic trash accumulation. Note, that once this is
> implemented, all Py_DECREF and PY_CLEAR in the _dealloc methods need to
> replaced by Py_XDECREF , as things may get null-ed by tp_clear, before
> invocation of dealloc.

As noted above, Py_CLEAR() is GC safe.

Also, for immutable objects that don't cause cycles on allocation it
is generally safe to go without a tp_clear() method.  As an example,
if an object holds a reference to a dictionary, there is no way to
change which dictionary is pointed to and the dictionary is allocated
by the constructor, then you don't need a tp_clear() to kill that
reference: the other objects in a potential cycle (e.g. the dictionary
itself) are responsible.

The async_cursor bit does sound like a cycle we should handle though.

> 3. Wherever the code creates or deletes python objects (such as list, dict,
> string etc), other than in the constructors and desctructors, the code
> should acquire the GIL before doing so. I think this is a general guideline
> for the python C/API (I remember seeing this somewhere, but am unable to
> find an online reference now.). This is because any python operation can
> lead to a garbage collection run (Gargabe collection runs get triggered when
> the difference between the number of python object allocs and deallocs
> reaches a certain value) and it needs to be ensured that there is atmost
> only one thread running the garbage collector at a time.

In C function/method calls, the GIL is acquired by default so things
are probably safer than you make out.  Such manipulation functions
need to be GC safe if they're decrementing reference counts though.
Py_CLEAR() the old value, incref the new value before assigning to the
pointer.  Alternatively, untracking and retracking the object is a big
hammer that can be used to avoid the problem.

James.