[vtk-developers] Handling invalid vtp files

[Francois Bertel]

On Sun, Feb 14, 2010 at 2:01 PM, Berk Geveci <berk.geveci at kitware.com> wrote:
> I agree with Francois and Karthik :-)
>
> I think that adding the assert Karthik suggested is a good idea even if we
> do proper input checking in the reader. If someone calls GetTuple() with an
> out-of-bounds index, the program is likely to misbehave or crash so  we
> might as well let the developer (not the end-user since asserts are compiled
> out in release builds) know that there is a bug somewhere.
>
> I think that's a pre-condition. Is that right Francois?

The assertion I suggested (bound checking) is a pre-condtion.

Karthik's assertion is not a pre-condition: a pre-condition should be
checkable by the caller prior entering the method. Conceptually,
pre-conditions are part of the interface of the method interface (in
C++ you cannot enforce that, but in the Eiffel language, this is
enforced by a specific syntax).

Karthik's assertion uses t, a pointer computed inside the method from
protected ivars, not accessible to the caller.
In the case of this->Array is NULL, what happen if i is not 0?

 T* t = this->Array + this->NumberOfComponents*i;

I don't think that t is still NULL. Karthik's assertion will pass
then, but the next statement t[j] will crash.


In general if i is out-of-bound, t will return an invalid pointer,
either a pointer to another allocated memory section (and t[j]  will
no crash and just give random value) or a pointer to a none-allocated
memory section (and t[j] will crash.).

Is it possible that GetNumberOfTuples() is not 0 when Array is NULL?
If not, then the pre-condition I suggested deal with all the cases
listed above.


An assertion placed anywhere does not give you much information about
the reason why it could fail (specially if there is no comment coming
with it). On the contrary, a failing pre-condition gives you two
pieces of information: 1.what condition failed and 2. which piece of
code is at fault:
the caller (or some caller above in the stack)


A bug has 2 aspects:
1. it is a failure (ie a visible "symptom", for example, a crash, a
wrong result, a test not passing)
2. and it is a fault (or a set of faults) in the source code.

Fixing a bug is 3 things:
1. locating the fault in the source code (ie where is the fault?),
2. understanding the fault (ie why is it a fault?)
3. and fixing the fault (how to fix it?).

Fixing the fault is usually a fast process once you understand the
fault. What *cost the most* in fixing a bug is actually *locating* the
faulty piece of code.

When a pre-condition fails you correlate failure and fault reducing
drastically the time you spend locating a fault. This is the power of
programming by contract.


>
> -berk
>
> On Sun, Feb 14, 2010 at 10:47 AM, Francois Bertel
> <francois.bertel at kitware.com> wrote:
>>
>> Asserts are for programming errors ONLY. Assert check if a programming
>> is valid, bug free. It is just a very basic way in C++ to implement
>> Programming by Contract
>> http://en.wikipedia.org/wiki/Design_by_contract (check of
>> pre/post-conditions and invariants).
>>
>>
>> They are NOT for checking the validity of the incoming data a program
>> is loading or receiving. A program has to be robust regarding the
>> input data (a file can be badly generated or can be corrupted because
>> of a faulty block on a disk or an error in a remote connexion).
>>
>>
>>
>> On Sun, Feb 14, 2010 at 8:58 AM, Karthik Krishnan
>> <karthik.krishnan at kitware.com> wrote:
>> > On Sat, Feb 13, 2010 at 6:18 PM, David Doria <daviddoria+vtk at gmail.com>
>> > wrote:
>> >> to something like:
>> >> template <class T>
>> >> void vtkDataArrayTemplate<T>::GetTuple(vtkIdType i, double* tuple)
>> >> {
>> >>   T* t = this->Array + this->NumberOfComponents*i;
>> >>   if(!t)
>> >>     {
>> >>     return;
>> >>     }
>> >
>> > You could change that to : assert(t != NULL)
>> >
>> > I think the VTK community would okay assert's. They compile only when
>> > _DEBUG is defined; ie no overhead in the release version. That said,
>> > Berk is right, that in this case it doesn't provide very useful
>> > information..
>> >
>> > Mimicking some of boost's compile time and static assertion's might be
>> > good for VTK, to enable bounds checking and traits validity even in
>> > performance critical code.
>> > _______________________________________________
>> > Powered by www.kitware.com
>> >
>> > Visit other Kitware open-source projects at
>> > http://www.kitware.com/opensource/opensource.html
>> >
>> > Follow this link to subscribe/unsubscribe:
>> > http://www.vtk.org/mailman/listinfo/vtk-developers
>> >
>> >
>>
>>
>>
>> --
>> François Bertel, PhD  | Kitware Inc. Suite 204
>> 1 (518) 371 3971 x113 | 28 Corporate Drive
>>                      | Clifton Park NY 12065, USA
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>
>
>



-- 
François Bertel, PhD  | Kitware Inc. Suite 204
1 (518) 371 3971 x113 | 28 Corporate Drive
                      | Clifton Park NY 12065, USA