[Cython] segfault due to using DECREF instead of XDECREF

[Victor Makarov]

2014/1/23 Syam Gadde <syam.gadde at duke.edu>:
> On 01/23/2014 01:04 PM, Stefan Behnel wrote:
>>
>> BTW, thanks for cutting down the code to a short and easy to analyse test
>> case, but if the original code really has a control flow like that, there
>> might be some space left for improved clarity.
>
>
> Absolutely.  In the wild, it manifested itself in PyFFTW.  It turns out I
> will likely not be using it, but thought it was an interesting edge case,
> and was what looked like valid Python, though it plays hard and fast with
> Python's runtime binding...
>
> -syam
>
>
> On 01/23/2014 01:04 PM, Stefan Behnel wrote:
>>
>> Syam Gadde, 21.01.2014 23:00:
>>>
>>> It seems that cython's static code analysis is failing on the following
>>> code.  It thinks that a particular variable (myobject2) must not be
>>> NULL, but in fact it is, so the __Pyx_DECREF_SET segfaults.  This came
>>> from third-party code, so I could fix it with an explicit initialization
>>> to None, but I'd love to have Cython deal with it directly.  Thanks for
>>> your help!
>>>
>>> # When compiled with Cython 0.19.2 or 0.20dev, the following code
>>> # segfaults on import
>>>
>>> import random
>>>
>>> def myfunc():
>>>       ## uncommenting the following fixes things
>>>       #myobject2 = None
>>>
>>>       myfalse = random.random() > 2
>>>
>>>       if myfalse:
>>>           myobject = None
>>>           myobject2 = None
>>>
>>>       if not myfalse:
>>>           # here Cython uses __Pyx_XDECREF_SET
>>>           myobject = None
>>>
>>>       print "Made it past myobject assignment"
>>>
>>>       if not myfalse or myobject2 is None:
>>>           # here Cython uses Pyx_DECREF_SET because it has determined
>>>           # that __pyx_v_myobject2 can't be null, but it really, really
>>> can!
>>>           # (if no one assigned myobject2 yet)
>>>           myobject2 = None
>>>
>>>       print "Made it past myobject2 assignment"
>>>
>>> myfunc()
>>
>> Thanks for the report. The generated C code currently looks like this:
>>
>> ===================
>> /*
>>   *      if not myfalse or myobject2 is None:             # <<<<<<<<<<<<<<
>>   */
>>    __pyx_t_4 = __Pyx_PyObject_IsTrue(__pyx_v_myfalse); if /*error*/
>>    __pyx_t_3 = ((!__pyx_t_4) != 0);
>>    if (!__pyx_t_3) {
>>      if (unlikely(!__pyx_v_myobject2)) {
>>                       __Pyx_RaiseUnboundLocalError("myobject2"); {...} }
>>      __pyx_t_4 = (__pyx_v_myobject2 == Py_None);
>>      __pyx_t_5 = (__pyx_t_4 != 0);
>>    } else {
>>      __pyx_t_5 = __pyx_t_3;
>>    }
>>    if (__pyx_t_5) {
>>
>> /*
>>   *          myobject2 = None             # <<<<<<<<<<<<<<
>>   */
>>      __Pyx_INCREF(Py_None);
>>      __Pyx_DECREF_SET(__pyx_v_myobject2, Py_None);
>> ===================
>>
>> So, it's smart enough to figure out that it has to raise an
>> UnboundLocalError on the "if" test if the variable hasn't been set yet,
>> but
>> then fails to see that in the case that the condition short-circuits, this
>> test hasn't been executed yet.
>>
>> BTW, thanks for cutting down the code to a short and easy to analyse test
>> case, but if the original code really has a control flow like that, there
>> might be some space left for improved clarity.
>>
>> Stefan
>>


Hi!

It's a bug! Being referenced in expression varaiable is marked as not
null. But it doesn't take into account that some subexpressions wasn't
evaluated. Here is another example:

def crashme(a=True):
    if not a:
        b = []
    (a or b) and (b or a)
crashme()

It seems to me that reference shouldn't affect variable's flags since
we don't have a simple way to track evaluation order.

I'd like to fix this if you don't mind.


-- 
vitja.