headers
Andrew Straw | 20 Aug 11:04
Gravatar

[Cython] patch for bad buffer creation

Hi,

I tracked down an issue that was giving me segfaults with a poor 
__getbuffer__ implementation and modified the compiler to catch this case.

I couldn't figure out how to create an account on Trac, so instead of 
submitting a patch there, I'm just sending this email. Included is a 
testcase added to tests/run/bufaccess.pyx. This is a patch against 
cython-devel and I release this under the PSF license (like the rest of 
Cython).

I had to use "hg diff -b" to create this patch while ignoring whitespace 
because I have emacs configured to strip trailing whitespace, which 
caused lots of false positives on the code. So, hopefully, this patch 
will still apply cleanly.

(The patch also includes a tiny fix to the ErrorBuffer implementation in 
tests/run/bufaccess.pyx.)

-Andrew
Attachment (bad_buffer_implementation_check.patch): text/x-diff, 3035 bytes
Hi,

I tracked down an issue that was giving me segfaults with a poor 
__getbuffer__ implementation and modified the compiler to catch this case.

I couldn't figure out how to create an account on Trac, so instead of 
submitting a patch there, I'm just sending this email. Included is a
(Continue reading)

Permalink | Reply |
headers
Dag Sverre Seljebotn | 20 Aug 12:17

Re: [Cython] patch for bad buffer creation

Andrew Straw wrote:
> Hi,
> 
> I tracked down an issue that was giving me segfaults with a poor 
> __getbuffer__ implementation and modified the compiler to catch this case.

Background first:

The situation is that this patch guards against returning erronous data 
from a GetBuffer call. According to the PEP (3118), I think it is pretty 
clear that data should either be set or an exception raised, so this 
patch only helps detect when you create an invalid __getbuffer__ 
implementation.

(If I am wrong and the PEP says that the buf field can be set to NULL on 
return then I'll of course apply the patch!)

I'd like input from others on this before making up my mind permanently, 
but I'm -1 so far. The reason is that this belongs in a unit test that 
you would write for your new buffer implementation, without adding 
another redundtant if-test to all Cython buffer acquisitions that works 
against correct getbuffer implementations.

With a unit test like that, you avoid runtime penalty everywhere, and 
you also get to test strides, shape etc. for which you cannot insert 
validation in a generic way in Cython (and so your patch check for 
correct "buf" but not correct "strides" or "shape").

So what I would propose instead (though I'll never get time for it :-) ) 
is to have a reusable "buffer tester" library, i.e. use Cython to create
(Continue reading)

Permalink | Reply |
headers
Dag Sverre Seljebotn | 20 Aug 12:20

Re: [Cython] patch for bad buffer creation

Dag Sverre Seljebotn wrote:
> Andrew Straw wrote:
>> Hi,
>>
>> I tracked down an issue that was giving me segfaults with a poor 
>> __getbuffer__ implementation and modified the compiler to catch this case.
 >
> I'd like input from others on this before making up my mind permanently, 
> but I'm -1 so far. The reason is that this belongs in a unit test that 

*However*, thanks a lot for creating the patch and bringing up the 
issue! (And I don't make the final call here either, it may go in.)

--

-- 
Dag Sverre
Permalink | Reply |
headers
Andrew Straw | 20 Aug 12:52
Gravatar

Re: [Cython] patch for bad buffer creation

Dag Sverre Seljebotn wrote:
> Andrew Straw wrote:
>   
>> Hi,
>>
>> I tracked down an issue that was giving me segfaults with a poor 
>> __getbuffer__ implementation and modified the compiler to catch this case.
>>     
>
> Background first:
>
> The situation is that this patch guards against returning erronous data 
> from a GetBuffer call. According to the PEP (3118), I think it is pretty 
> clear that data should either be set or an exception raised, so this 
> patch only helps detect when you create an invalid __getbuffer__ 
> implementation.
>   
I understand your reasoning, but Cython's __getbuffer__ works outside 
the scope of PEP 3118. The PEP doesn't specify Cython behavior, and a 
great aspect of Cython, IMO, is that it makes Python/C bridges easier, 
not to adhere to the minimum standards specified by a PEP. Thus a small 
amount of additional error checking seems OK to me. Also, as another 
significant difference, Cython's implementation is working in Python 
earlier than 2.6, whereas the Python development process will only bring 
this out with 2.6. (I'm certainly not complaining -- it's just to point 
out that the PEP doesn't directly apply.)
> (If I am wrong and the PEP says that the buf field can be set to NULL on 
> return then I'll of course apply the patch!)
>
> I'd like input from others on this before making up my mind permanently,
(Continue reading)

Permalink | Reply |
headers
Dag Sverre Seljebotn | 20 Aug 14:58

Re: [Cython] patch for bad buffer creation

Hi,

this discussion was interesting. While I may seem to have en edge below 
I mean it all for the sake of the discussion and hope we can continue to 
have this "in a good spirit".

(Inserting this note was easier than redoing my mail. I'm excited, not 
angry :-) )

Andrew Straw wrote:
> Dag Sverre Seljebotn wrote:
>   
>> Andrew Straw wrote:
>>   
>>     
>>> Hi,
>>>
>>> I tracked down an issue that was giving me segfaults with a poor 
>>> __getbuffer__ implementation and modified the compiler to catch this case.
>>>     
>>>       
>> Background first:
>>
>> The situation is that this patch guards against returning erronous data 
>> from a GetBuffer call. According to the PEP (3118), I think it is pretty 
>> clear that data should either be set or an exception raised, so this 
>> patch only helps detect when you create an invalid __getbuffer__ 
>> implementation.
>>   
>>
(Continue reading)

Permalink | Reply |
headers
Robert Bradshaw | 20 Aug 20:58

Re: [Cython] patch for bad buffer creation

On Wed, 20 Aug 2008, Dag Sverre Seljebotn wrote:

> Hi,
>
> this discussion was interesting. While I may seem to have en edge below
> I mean it all for the sake of the discussion and hope we can continue to
> have this "in a good spirit".

Yes, me too. Buffer support is a huge and wonderful feature, but given its 
scope and newness I am sure there is room for improvement. Thanks for 
bringing this up.

> (Inserting this note was easier than redoing my mail. I'm excited, not
> angry :-) )
>
> Andrew Straw wrote:
>> Dag Sverre Seljebotn wrote:
>>
>>> Andrew Straw wrote:
>>>
>>>
>>>> Hi,
>>>>
>>>> I tracked down an issue that was giving me segfaults with a poor
>>>> __getbuffer__ implementation and modified the compiler to catch this case.
>>>>
>>>>
>>> Background first:
>>>
>>> The situation is that this patch guards against returning erronous data
(Continue reading)

Permalink | Reply |
headers
Andrew Straw | 21 Aug 14:50
Gravatar

Re: [Cython] patch for bad buffer creation

Robert Bradshaw wrote:

>>> Dag Sverre Seljebotn wrote:
>>>
>>>> Andrew Straw wrote:

>>> great aspect of Cython, IMO, is that it makes Python/C bridges easier,
>>> not to adhere to the minimum standards specified by a PEP. Thus a small
>>> amount of additional error checking seems OK to me. Also, as another
>>>
>> Well, in this case, the error check should be done by inserting some
>> code automatically in the definition of __getbuffer__, so that Python 3
>> consumers also get advantage of it, and so that we don't second guess
>> how good non-Cython implementations are. So the check is still inserted
>> in the wrong place.
> 
> I agree, this is the right place to put the check. And I think it is very 
> worth the (relatively small) overhead to insert a check here.

Just to be clear (re-reading this bit after Robert's response makes me 
realize I'm confused about your meaning): what do you mean by "the 
definition of __getbuffer__" -- the C Python implementation of 
PyObject_GetBuffer()? I thought my patch did affect the (Cython) 
definition of __getbuffer__. Also, wouldn't Cython emit the same C code 
  for Python 3?

-Andrew
Permalink | Reply |
headers
Robert Bradshaw | 21 Aug 18:14

Re: [Cython] patch for bad buffer creation

On Aug 21, 2008, at 5:50 AM, Andrew Straw wrote:

> Robert Bradshaw wrote:
>
>>>> Dag Sverre Seljebotn wrote:
>>>>
>>>>> Andrew Straw wrote:
>
>>>> great aspect of Cython, IMO, is that it makes Python/C bridges  
>>>> easier,
>>>> not to adhere to the minimum standards specified by a PEP. Thus  
>>>> a small
>>>> amount of additional error checking seems OK to me. Also, as  
>>>> another
>>>>
>>> Well, in this case, the error check should be done by inserting some
>>> code automatically in the definition of __getbuffer__, so that  
>>> Python 3
>>> consumers also get advantage of it, and so that we don't second  
>>> guess
>>> how good non-Cython implementations are. So the check is still  
>>> inserted
>>> in the wrong place.
>>
>> I agree, this is the right place to put the check. And I think it  
>> is very
>> worth the (relatively small) overhead to insert a check here.
>
> Just to be clear (re-reading this bit after Robert's response makes me
> realize I'm confused about your meaning): what do you mean by "the
(Continue reading)

Permalink | Reply |
headers
Andrew Straw | 21 Aug 18:30
Gravatar

Re: [Cython] patch for bad buffer creation

Robert Bradshaw wrote:
> On Aug 21, 2008, at 5:50 AM, Andrew Straw wrote:
>
>   
>> Robert Bradshaw wrote:
>>
>>     
>>>>> Dag Sverre Seljebotn wrote:
>>>>>
>>>>>           
>>>>>> Andrew Straw wrote:
>>>>>>             
>>>>> great aspect of Cython, IMO, is that it makes Python/C bridges  
>>>>> easier,
>>>>> not to adhere to the minimum standards specified by a PEP. Thus  
>>>>> a small
>>>>> amount of additional error checking seems OK to me. Also, as  
>>>>> another
>>>>>
>>>>>           
>>>> Well, in this case, the error check should be done by inserting some
>>>> code automatically in the definition of __getbuffer__, so that  
>>>> Python 3
>>>> consumers also get advantage of it, and so that we don't second  
>>>> guess
>>>> how good non-Cython implementations are. So the check is still  
>>>> inserted
>>>> in the wrong place.
>>>>         
>>> I agree, this is the right place to put the check. And I think it
(Continue reading)

Permalink | Reply |
headers
Dag Sverre Seljebotn | 21 Aug 18:45

Re: [Cython] patch for bad buffer creation

Andrew Straw wrote:
> Robert Bradshaw wrote:
>> On Aug 21, 2008, at 5:50 AM, Andrew Straw wrote:
>>
>>
>>> Robert Bradshaw wrote:
>>>
>>>
>>>>>> Dag Sverre Seljebotn wrote:
>>>>>>
>>>>>>
>>>>>>> Andrew Straw wrote:
>>>>>>>
>>>>>> great aspect of Cython, IMO, is that it makes Python/C bridges
>>>>>> easier,
>>>>>> not to adhere to the minimum standards specified by a PEP. Thus
>>>>>> a small
>>>>>> amount of additional error checking seems OK to me. Also, as
>>>>>> another
>>>>>>
>>>>>>
>>>>> Well, in this case, the error check should be done by inserting some
>>>>> code automatically in the definition of __getbuffer__, so that
>>>>> Python 3
>>>>> consumers also get advantage of it, and so that we don't second
>>>>> guess
>>>>> how good non-Cython implementations are. So the check is still
>>>>> inserted
>>>>> in the wrong place.
>>>>>
(Continue reading)

Permalink | Reply |
headers
Dag Sverre Seljebotn | 21 Aug 18:49

Re: [Cython] patch for bad buffer creation

Dag Sverre Seljebotn wrote:
> 2. Have a look at Nodes.FuncDefNode.generate_function_definition and
> insert a check for self.name == "__getbuffer__" (and somehow a check that
> you are in a cdef class, not sure, well, at least
> "self.scope.parent_scope" should be a cdef class scope, for scopes see
> Symtab.py), and if it matches, then simply output the C code at the right
> place (after the label that is jumped to for return statements).

Sorry, that should be self.local_scope, and I'm not entirely sure about
the name of the enclosing scope (but something like owner_scope or
parent_scope, see in Symtab.py).

Dag Sverre
Permalink | Reply |
headers
Stefan Behnel | 21 Aug 19:22

Re: [Cython] patch for bad buffer creation

Hi,

Andrew Straw wrote:
> OK, so there'd be part of the Cython compiler that checked if the name 
> of the current method was "__getbuffer__" and if the class was an 
> extension ctypedef, upon which it would insert some check before the 
> method returned? I don't know the internals of Cython enough to know 
> whether that would be a gratuitous hack or reasonable behavior...

:) well, Cython generates the C code, so it actually sees the pointer that
this special method returns. It's trivial to put some additional sanity check
code in the final cleanup code that gets generated for that method.

> It does seem like the idea of a "training wheels" mode for Cython could
> be pretty useful -- a few of the other threads are mentioning various
> ideas, and it would probably make development of Cython code easier if
> there was a safe(r) but slower mode and a as fast as possible mode.

Maybe you could start a Wiki page on this topic?

http://wiki.cython.org/enhancements/safetymode

Stefan
Permalink | Reply |
headers
Dag Sverre Seljebotn | 25 Aug 17:13

Re: [Cython] patch for bad buffer creation

Andrew Straw wrote:
> Robert Bradshaw wrote:
> OK, so there'd be part of the Cython compiler that checked if the name 
> of the current method was "__getbuffer__" and if the class was an 
> extension ctypedef, upon which it would insert some check before the 
> method returned? I don't know the internals of Cython enough to know 
> whether that would be a gratuitous hack or reasonable behavior... But if 
> you point me in the right direction, I'll have a look.

This just got a lot easier in the latest cython-devel to do if you want 
to have a shot.

If you look around line 1065 in Cython/Compiler/Nodes.py 
(FuncDefNode.getbuffer_X) you can see special functions which are called 
to insert code only if we're dealing with __getbuffer__. You could 
insert code to check if buf is non-NULL in the normal_cleanup case. If 
you need auxiliary code for the exception you can call 
code.globalscope.use_utility_code (see how it is done elsewhere).

--

-- 
Dag Sverre
Permalink | Reply |
headers
Robert Bradshaw | 20 Aug 20:47

Re: [Cython] patch for bad buffer creation

On Wed, 20 Aug 2008, Andrew Straw wrote:

> Hi,
>
> I tracked down an issue that was giving me segfaults with a poor 
> __getbuffer__ implementation and modified the compiler to catch this case.
>
> I couldn't figure out how to create an account on Trac,

Anonymous account creation is disable only because it's the most effective 
method (so far) of blocking spam. Feel free to email me oflist with an 
.htpasswd file (or desired username/password) and I'll create an account 
for you.

> so instead of 
> submitting a patch there, I'm just sending this email. Included is a testcase 
> added to tests/run/bufaccess.pyx. This is a patch against cython-devel and I 
> release this under the PSF license (like the rest of Cython).
>
> I had to use "hg diff -b" to create this patch while ignoring whitespace 
> because I have emacs configured to strip trailing whitespace, which caused 
> lots of false positives on the code. So, hopefully, this patch will still 
> apply cleanly.

Thanks. I see Dag has replied, I'm going to go read his comments.

> (The patch also includes a tiny fix to the ErrorBuffer implementation in 
> tests/run/bufaccess.pyx.)
>
> -Andrew
(Continue reading)

Permalink | Reply |

Gmane