[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...

On Fri, Jun 20, 2008 at 11:48 AM, Burlen Loring <burlen.loring at kitware.com>

> 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
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