Potential problems for extern cdefs in argument parsing about cython HOT 10 CLOSED

@robertwb commented

I haven't been able to use this to produce a crash on my machine, but it does look bad...

from cython.

@robertwb changed milestone to 0.9.8.2
commented

Lets try to come up with some specific examples here...

from cython.

@anonymous changed resolution to fixed
status from new to closed
commented

Without further verification, this problem is most likely resolved by the new argument parsing code.

from cython.

@dagss changed milestone from 0.9.8.2 to wishlist
priority from major to minor
resolution from fixed to empty
status from closed to reopened
commented

The danger of a crash is now gone but there is still some inconsistency within Cython whether the size of the type plays a role for external typedefs, and I'd like the behaviour to change here rather than other places personally. Reopening but making it a minor wishlist issue.

Details:

Does anyone have any thoughts about the external typedef issue with 
argument unpacking? I see that with Stefan's code we get an exception 
rather than a segfault which is a big step forward, but one could still 
think a bit about what the behaviour should be (i.e. if people wants to 
use numpy.uint16_t as a function argument type I want a defined 
behaviour for that.)

Example:

test.h:
typedef long int my_int;

test.pyx:

cdef extern from "test.h":
    ctypedef char myint

def myfunc(myint x):
    pass

Now, myfunc(1000) will raise an overflow exception.

Granted, this is an extreme example. I just want a defined behaviour on 
this somehow, and documenting the existing one could be ok. If we want 
to support it without assumptions on the size, something like

#define __Pyx_FetchPyInt(result, from) switch(sizeof(result)) {\
  case sizeof(char): result = __pyx_PyInt_char(from); break;
  case sizeof(int):
  ....
   default: __Pyx_RaiseFetchPyIntError(from); result = -1;
}

should do the trick (and it is only needed when the typedef is extern). 
Will a patch like that be accepted? (It will have to wait a month or two 
if I'm the one doing it, but I am not expecting anybody else to in the 
meantime.)

from cython.

@robertwb commented

It should be noted that argument parsing has been entirely re-written and avoids explicit calls to PyArg_ParseTupleAndKeywords in most cases.

from cython.

scoder commented

We can make that "all cases" now.

from cython.

scoder changed milestone from wishlist to 0.11
resolution to fixed
status from reopened to closed
commented

no idea when exactly this was fixed (is that still a problem in 0.10.x?), but argument parsing has its own unpacking code now, so this is no longer an issue.

from cython.

scoder changed resolution from fixed to empty
status from closed to reopened
commented

from cython.

scoder changed owner from somebody to scoder, robertwb
status from reopened to new
commented

from cython.

scoder changed resolution to fixed
status from new to closed
commented

from cython.