[vtk-developers] vtkDataArrayTemplate.cxx writes out of bounds when malloc fails
Will Schroeder
will.schroeder at kitware.com
Fri Jun 20 11:56:55 EDT 2008
In the long run, and this is probably a dumb idea, we may want to take over
heap management. There are a lot of reasons for this, performance, error
handling, etc., but this is a big bite to take...
W
On Fri, Jun 20, 2008 at 11:48 AM, Burlen Loring <burlen.loring at kitware.com>
wrote:
> Utkarsh,
>
> Utkarsh Ayachit wrote:
>
>> In the new "new" implementation a resize always needs atleast twice as
>> much memory available.
>>
> That is actually how the existing implementation works... On the bright
> side, I think its also the default for how stl vectors are implemented...and
> gives the new "new" implementation a realloc like characteristic. In any
> case, my intuition is that using new will be slower than malloc but easier
> to achieve portable reliability...
>
> Can anyone suggest "VTK approved" alternatives?
>
> Obviously we can't continue to write into memory we don't own, but how can
> we portably, reliably and efficiently let the caller know that an
> InsertTuple, GetWritePointer has failed due to failed malloc?
>
> Burlen
>
>
>
> Utkarsh Ayachit wrote:
>
>> The nice thing about realloc is that even when the memory grows, it may
>> not result in any memory moves. In the new "new" implementation a resize
>> always needs atleast twice as much memory available. With realloc, if the
>> original memptr is already at the end of the heap (for example), then
>> realloc will simply "grow" the data block allocated, without "moving" the
>> block at all -- hence no copy incurred. Which also makes it possible to grow
>> the array to a larger size than possible with "new|copy|delete" for a given
>> system.
>>
>> Utkarsh
>>
>> Burlen Loring wrote:
>>
>>> Utkarsh,
>>>
>>> Oh, OK, sorry Dave I misunderstood.
>>>
>>> In the current implementation, if __APPLE__ is defined then realloc is
>>> *not* used. Arrays are always moved. From what I understand(I am not a mac
>>> user) gcc defines this on mac. & gcc is mac's default compiler. I assume
>>> that if not using realloc were a huge issue that mac users would have
>>> complained about this? Also we have the choice of doing something like:
>>>
>>> if NewSize<OldSize
>>> update size
>>> update end of array index
>>> else
>>> get new memory
>>> copy
>>> delete old memory
>>> update size
>>> update end of array index
>>> fi
>>> in our Realloc. I didn't bother to do this because, of my point about
>>> the mac. If we did this it would have to be up to the user to "squeeze" in
>>> order to reclaim the wasted bytes.
>>>
>>> Burlen
>>>
>>>
>>> Utkarsh Ayachit wrote:
>>>
>>>> Burlen,
>>>>
>>>> I think what Dave is trying to say is that now since you cannot use
>>>> realloc, every time the resize happens, it will have to be a new-and-copy
>>>> operation. With realloc it is possible that the memory block is never moved.
>>>> I think now I remember why I had reverted the malloc-to-new commit in past
>>>> -- it was due to same realloc issue.
>>>>
>>>> Utkarsh
>>>>
>>>> Burlen Loring wrote:
>>>>
>>>>> Hi David,
>>>>>
>>>>> Maybe I should have gone into more detail in my description of the
>>>>> changes. In this class there is a flag to indicate what the "delete method"
>>>>> is as a user can pass in an array to use. They can set either to use free or
>>>>> delete. So what I have done in the line you reference is use realloc if the
>>>>> array has been marked as created with malloc. This is how the class behaved
>>>>> before.
>>>>>
>>>>> Burlen
>>>>>
>>>>> David Cole wrote:
>>>>>
>>>>>> Well, for one thing, you can't do this:
>>>>>> + newArray = static_cast<T*>(realloc(this->Array, sz*sizeof(T)));
>>>>>>
>>>>>> if you eliminate all the use of malloc. realloc calls don't make any
>>>>>> sense if you are using C++ new/delete.
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2008/6/19 Burlen Loring <burlen.loring at kitware.com <mailto:
>>>>>> burlen.loring at kitware.com>>:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I would like to get some feed back on a change I would like to
>>>>>> make to vtkDataArrayTemplate.txx.
>>>>>>
>>>>>> Here is the problem that prompted the changes:
>>>>>>
>>>>>> Currently during inserts, a data array resizes itself if need be.
>>>>>> Unfortunately, in the case that it runs out of memory during the
>>>>>> resize, it ignores the fact that it didn't get the needed space
>>>>>> and writes into memory it doesn't own. For example take a look at
>>>>>> how GetWritePointer is ignoring the return value from
>>>>>> ResizeAndExtend which, will be 0 to indicate that malloc failed.
>>>>>> And further how InsertNextTuple/InsertTuple has no way of knowing
>>>>>> if the malloc failed. The insert then takes place into memory not
>>>>>> owned by the data array.
>>>>>>
>>>>>> If you are unfortunate enough to use enough ram that malloc fails
>>>>>> during a series of inserts, this can cause some very strange
>>>>>> crashes!
>>>>>>
>>>>>> Attached is a patch I'd like to submit to cvs. What I have done is
>>>>>> replace the malloc/free pairs with new/delete pairs. Failure of
>>>>>> new is much easier to deal with (compared to malloc) because it
>>>>>> throws a bad_alloc exception, which clearly indicates the nature
>>>>>> of the problem, and the end user can decide how to handle that.
>>>>>> Additionally the patch makes sure that if new returns 0 rather
>>>>>> than throwing an exception(could happen on older compilers), a
>>>>>> return value indicating an error occurred gets propagated back up
>>>>>> to the caller immediately. For methods that return pointers the
>>>>>> error value is 0 for methods that return vtkIdType the error value
>>>>>> is -1. There are a few methods (eg SetTuple) that are typed as
>>>>>> returning void, which simply return. In the case of new failures
>>>>>> during an insert, this prevents the data array from writing into
>>>>>> memory it doesn't own.
>>>>>>
>>>>>> Any objections/criticism/comments regarding this change?
>>>>>>
>>>>>> Thanks in advance
>>>>>> Burlen
>>>>>>
>>>>>> -- Burlen Loring
>>>>>> Kitware, Inc.
>>>>>> R&D Engineer
>>>>>> 28 Corporate Drive
>>>>>> Clifton Park, NY 12065-8662
>>>>>> Phone: 518-371-3971 x137
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> vtk-developers mailing list
>>>>>> vtk-developers at vtk.org <mailto:vtk-developers at vtk.org>
>>>>>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>
> --
> Burlen Loring
> Kitware, Inc.
> R&D Engineer
> 28 Corporate Drive
> Clifton Park, NY 12065-8662
> Phone: 518-371-3971 x137
>
> _______________________________________________
> vtk-developers mailing list
> vtk-developers at vtk.org
> http://www.vtk.org/mailman/listinfo/vtk-developers
>
--
William J. Schroeder, PhD
Kitware, Inc.
28 Corporate Drive
Clifton Park, NY 12065
will.schroeder at kitware.com
http://www.kitware.com
518-371-3971 (phone and fax)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20080620/dd053394/attachment.html>
More information about the vtk-developers
mailing list