[vtk-developers] Safer item access in Python wrapped VTK arrays

Wed Aug 23 08:45:45 EDT 2017

The MR is now complete, it covers vtkPoints and the vtkDataArray classes,
along with vtkStringArray.
https://gitlab.kitware.com/vtk/vtk/merge_requests/3176

<https://gitlab.kitware.com/vtk/vtk/merge_requests/3176>Adding a
VTK_EXPECTS(condition) check to a VTK method causes a ValueError exception
to be raised if the condition is not satisfied when the method is called.
There was some discussion on the MR about whether a failed index check
(e.g. for GetValue(id)) should raise IndexError instead of ValueError.
Since VTK_EXPECTS() is just a generic check on the parameter values, I've
stuck with ValueError so far.  In addition to checking index values,
VTK_EXPECTS() is also used to check for NULL pointers in calls where they
aren't permitted.

Any comments are welcome.

Cheers,
  - David

On Sun, Aug 20, 2017 at 11:04 PM, David Gobbi <david.gobbi at gmail.com> wrote:

> I've put together an MR that uses the VTK_EXPECTS() macro to add Python
> safety checks to vtkPoints:
> https://gitlab.kitware.com/vtk/vtk/merge_requests/3176
>
> If the point Id for GetPoint() or SetPoint() is out of range, then Python
> will raise an exception instead of crashing.
>
>  - David
>
>
> On Thu, Aug 10, 2017 at 12:44 AM, Elvis Stansvik <
> elvis.stansvik at orexplore.com> wrote:
>
>> Den 9 aug. 2017 10:54 em skrev "David Gobbi" <david.gobbi at gmail.com>:
>>
>> Here's what I'm currently thinking about for wrapper hints, please let me
>> know if the syntax is too obtrusive.
>>
>> The basic precondition contract, similar to proposed C++ contracts:
>>
>>     float GetValue(vtkIdType i)  VTK_EXPECTS(i >= 0 && i <
>> GetNumberOfValues());
>>
>>
>> This is really just a stylistic nitpick, and a matter of taste of course,
>> so take it or leave it, but I like it when the var is in between the
>> bounds, e.g.
>>
>>    0 <= i && i < GetNumberOfValues()
>>
>> As for the hint syntax, I also think what you last posted is readable
>> enough.
>>
>> Elvis
>>
>>
>> And here's the format that I'm currently thinking about for wrapper size
>> hints (to replace our "hints" file):
>>
>>     double *GetPoint(vtkIdType i)  VTK_SIZEHINT(return[3]);
>>
>> When used in combination things start to look ugly, but the method
>> signature is still readable:
>>
>>     void SetTuple(vtkIdType i, float *a)
>>         VTK_EXPECTS(i >= 0 && i < GetNumberOfTuples())
>>         VTK_SIZEHINT(a[GetNumberOfComponents()]);
>>
>>     float *GetTuple(vtkIdType i)
>>         VTK_EXPECTS(i >= 0 && i < GetNumberOfTuples())
>>         VTK_SIZEHINT(return[GetNumberOfComponents()]);
>>
>> Any thoughts?  I'd really like to express the size hint as some kind of
>> contract, but C++ doesn't provide a way to check the number of values
>> pointed to by a pointer.
>>
>> As an aside: for vtkDataArray, I don't actually intend to add
>> VTK_SIZEHINT as shown above.  Currently the size hints for vtkDataArray are
>> hard-coded into the wrappers, and I'll probably keep them that way in order
>> to keep the C++ headers clean.
>>
>>  - David
>>
>>
>> On Mon, Aug 7, 2017 at 6:56 AM, David Gobbi <david.gobbi at gmail.com>
>> wrote:
>>
>>> Hi Andras,
>>>
>>> I agree that these contracts are a better choice than inventing
>>> something of our own, I'm going to play around with this to see how easy it
>>> would be to add them to the wrappers.
>>>
>>> The macros can be made a little more compact:
>>> T& operator[](size_t i) VTK_PRECONDITION(i >= 0 && i < size());
>>>
>>> Or we could use VTK_EXPECTS(i >= 0 && i < size());
>>>
>>>  - David
>>>
>>>
>>>
>>> On Fri, Aug 4, 2017 at 7:39 PM, Andras Lasso <lasso at queensu.ca> wrote:
>>>
>>>> Eventually, C++17 standard contracts may be used to specify these
>>>> bounds checks. For now, we may use the same format but add them as comments
>>>> or protected by macros; and when we switch to C++17 then these can be
>>>> converted to actual contracts.
>>>>
>>>>
>>>>
>>>> Proposal for contracts: http://www.open-std.org/JTC1/S
>>>> C22/WG21/docs/papers/2015/n4415.pdf
>>>>
>>>>
>>>>
>>>> Example contract:
>>>>
>>>> T& operator[](size_t i) [[expects: i >= 0 && i < size()]];
>>>>
>>>>
>>>>
>>>> For now, we could do this in VTK:
>>>>
>>>> T& operator[](size_t i) VTK_CONTRACT([[expects: i >= 0 && i < size()]]);
>>>>
>>>>
>>>>
>>>> Andras
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> *From:* David Gobbi [mailto:david.gobbi at gmail.com]
>>>> *Sent:* Friday, August 4, 2017 4:05 PM
>>>> *To:* Berk Geveci <berk.geveci at kitware.com>
>>>> *Cc:* Andras Lasso <lasso at queensu.ca>; VTK Developers <
>>>> vtk-developers at vtk.org>
>>>> *Subject:* Re: [vtk-developers] Safer item access in Python wrapped
>>>> VTK arrays
>>>>
>>>>
>>>>
>>>> For a while, we kicked around the idea of having the hints in the
>>>> comments.  This would be less intrusive than adding C++11 attributes the
>>>> declarations (i.e. what I've been doing so far).
>>>>
>>>>
>>>>
>>>> The C++11 attributes are definitely the cleanest way for the wrappers
>>>> to store the hints, since the hints become part of the parse tree, so I
>>>> want to keep them as the primary hinting mechanism.  Comment-based hints
>>>> could be added as a secondary mechanism, i.e. after a declaration has been
>>>> parsed we can check the docstring for extra hints, and then add the hints
>>>> to the parse tree before the wrapper code is generated.
>>>>
>>>>
>>>>
>>>> I'll have to search through the archives to remember what kind of
>>>> syntax we were considering for comment-based hints.
>>>>
>>>>
>>>>
>>>>  - David
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Aug 4, 2017 at 1:30 PM, Berk Geveci <berk.geveci at kitware.com>
>>>> wrote:
>>>>
>>>> As much as I support the concept of range checking in the wrappers,
>>>> this hint will make things look very ugly and non-standard at a time where
>>>> we are trying to move towards more standard looking C++. Isn't there a way
>>>> of doing this without mangling the signatures?
>>>>
>>>>
>>>>
>>>> On Fri, Aug 4, 2017 at 1:06 PM, David Gobbi <david.gobbi at gmail.com>
>>>> wrote:
>>>>
>>>> Hi Andras,
>>>>
>>>>
>>>>
>>>> Yes, this would be possible.  It could be hard-coded into the wrappers,
>>>> or even better, in the header file a generic hint could be added to the
>>>> index parameter so that the index would be checked against the size of the
>>>> array:
>>>>
>>>>
>>>>
>>>>   float GetValue(vtkIdType idx VTK_RANGECHECK(0,GetNumberOfValues()));
>>>>
>>>>
>>>>
>>>> I have a list of wrapper hints at http://www.vtk.org/Wiki/VTK
>>>> /Wrapping_hints
>>>> <https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.vtk.org%2FWiki%2FVTK%2FWrapping_hints&data=02%7C01%7Classo%40queensu.ca%7C37f3749bcbbf4422f0ba08d4db741f07%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636374739163412038&sdata=FtQM8JHWrl8ReFwC3EFD4j0COBmWqm1r6k9b%2BF5tZzU%3D&reserved=0>,
>>>> and I can add this "RANGECHECK" hint to the "Proposed hints" section.
>>>>
>>>>
>>>>
>>>> I'm hoping to find time to implement more of these wrapper hints over
>>>> the next few weeks.
>>>>
>>>>
>>>>
>>>>  - David
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Aug 4, 2017 at 9:53 AM, Andras Lasso <lasso at queensu.ca> wrote:
>>>>
>>>> Hi David,
>>>>
>>>>
>>>>
>>>> Users who mostly use VTK via Python wrapping are sometimes surprised
>>>> how easily they can crash the application by accessing out-of-bounds
>>>> elements in arrays. For example, this crashes an application:
>>>>
>>>>
>>>>
>>>> >>> a=vtk.vtkStringArray()
>>>>
>>>> >>> print(a.GetValue(0))
>>>>
>>>>
>>>>
>>>> I first thought that it should be handled by documentation and
>>>> training, but it would be nicer if the wrappers could handle this.
>>>>
>>>>
>>>>
>>>> Would it be feasible to add bounds check to VTK array accessors in
>>>> Python wrappers, so that in case of an out of bounds access, a Python
>>>> exception would be raised instead of crashing the application?
>>>>
>>>> Andras
>>>>
>>>>
>>>>
>>>
>>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>>
>> Follow this link to subscribe/unsubscribe:
>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20170823/19ca53b4/attachment-0001.html>