Fix cython's gc_track and gc_untrack

[nbruin]

In a long sage-devel thread we eventually found in this message that a GC during a weakref callback on a Cython class can lead to double deallocation of that class. In Python's Objects/typeobject.c, line 1024 and onwards, there are some comments that indicate that earlier version of Python were bitten by this problem too. The solution is to insert the appropriate PyObject_GC_Untrack and PyObject_GC_Track in cython's deallocation code. This is best fixed in cython itself.

Install only the new spkg at http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.17.4.spkg

Upstream: Completely fixed; Fix reported upstream

CC: @simon-king-jena @jpflori

Component: memleak

Author: Robert Bradshaw

Reviewer: Jeroen Demeyer

Merged: sage-5.6.beta3

Issue created by migration from https://trac.sagemath.org/ticket/13896

[nbruin]

Patch to more reliably produce crash

[nbruin]

comment:1

Attachment: double-free-crash.patch.gz

With attached patch applied to 5.6.beta2 (and probably also other versions close to it),

sage -t devel/sage/sage/modules/module.pyx

will crash relatively reliably on several machines (including sage.math)

[jpflori]

comment:3

I'd like to see this ticket as a blocker, anyone against this idea?

[nbruin]

comment:4

Replying to @jpflori:

I'd like to see this ticket as a blocker, anyone against this idea?

Since this is the ultimate "can generate segfaults anywhere", it's a prime candidate for blocker status. However, we're fully at the mercy of cython developers as to when this gets fixed. Also, if we release with this bug unfixed, we might as well leave #715 in too, since this one has a much wider possible impact :-).

[jpflori]

comment:5

Ok, Ive put it as blocker.

For those who want to play while waiting for upstream, I've posted a p0 Cython spkg which does "something" with PyObject_GC_[Un]Track.
Not sure it makes any sense, but it seems to make our bug disappear.
It's at
http://boxen.math.washington.edu/home/jpflori/cython-0.17.3.p0.spkg

[nbruin]

Description changed:

--- 
+++ 
@@ -1 +1 @@
-In a long sage-devel thread we eventually found [in this message](https://groups.google.com/group/sage-devel/msg/1d05a46b9b5fa8e2?hl=en) that a GC during a weakref callback on a Cython class can lead to double deallocation of that class. In Python's [Objects/typeobject.h](http://svn.python.org/projects/python/trunk/Include/object.h),  line 1024 and onwards, there are some comments that indicate that earlier version of Python were bitten by this problem too. The solution is to insert the appropriate `PyObject_GC_Untrack` and `PyObject_GC_Track` in cython's deallocation code. This is best fixed in cython itself.
+In a long sage-devel thread we eventually found [in this message](https://groups.google.com/group/sage-devel/msg/1d05a46b9b5fa8e2?hl=en) that a GC during a weakref callback on a Cython class can lead to double deallocation of that class. In Python's [Objects/typeobject.c](http://svn.python.org/projects/python/trunk/Objects/typeobject.c),  line 1024 and onwards, there are some comments that indicate that earlier version of Python were bitten by this problem too. The solution is to insert the appropriate `PyObject_GC_Untrack` and `PyObject_GC_Track` in cython's deallocation code. This is best fixed in cython itself.

[nbruin]

comment:6

Apologies. I saw I linked to the wrong file. Include/object.h also has some interesting information, but it looks like it is a bit out-of-date on some bits. In particular, if you look at the actual use of the TRASHCAN macros:

    PyObject_GC_UnTrack(self);
    ++_PyTrash_delete_nesting;
    Py_TRASHCAN_SAFE_BEGIN(self);
    --_PyTrash_delete_nesting;
...
  endlabel:
    ++_PyTrash_delete_nesting;
    Py_TRASHCAN_SAFE_END(self);
    --_PyTrash_delete_nesting;

with the explanation a little lower:

       Q. Why the bizarre (net-zero) manipulation of
          _PyTrash_delete_nesting around the trashcan macros?

       A. Some base classes (e.g. list) also use the trashcan mechanism.
          The following scenario used to be possible:

          - suppose the trashcan level is one below the trashcan limit

          - subtype_dealloc() is called

          - the trashcan limit is not yet reached, so the trashcan level
        is incremented and the code between trashcan begin and end is
        executed

          - this destroys much of the object's contents, including its
        slots and __dict__

          - basedealloc() is called; this is really list_dealloc(), or
        some other type which also uses the trashcan macros

          - the trashcan limit is now reached, so the object is put on the
        trashcan's to-be-deleted-later list

          - basedealloc() returns

          - subtype_dealloc() decrefs the object's type

          - subtype_dealloc() returns

          - later, the trashcan code starts deleting the objects from its
        to-be-deleted-later list

          - subtype_dealloc() is called *AGAIN* for the same object

          - at the very least (if the destroyed slots and __dict__ don't
        cause problems) the object's type gets decref'ed a second
        time, which is *BAD*!!!

          The remedy is to make sure that if the code between trashcan
          begin and end in subtype_dealloc() is called, the code between
          trashcan begin and end in basedealloc() will also be called.
          This is done by decrementing the level after passing into the
          trashcan block, and incrementing it just before leaving the
          block.

          But now it's possible that a chain of objects consisting solely
          of objects whose deallocator is subtype_dealloc() will defeat
          the trashcan mechanism completely: the decremented level means
          that the effective level never reaches the limit.      Therefore, we
          *increment* the level *before* entering the trashcan block, and
          matchingly decrement it after leaving.  This means the trashcan
          code will trigger a little early, but that's no big deal.

It's probably better to leave out the trashcan for now. It seems like rather tricky code and I'm not sure it's part of the official Python C-API (it might be something internal, just like they use some macros themselves they find unsafe for use in extension modules)

[jpflori]

comment:7

I saw and read about this additional steps in addition to the macro, but I was not sure it was also needed here.

Anyway I agree it is a better take to leave that out for now, and anyway, upstream will decide what is the best.

So I've updated the spkg to not include the trashcan parts.