Deallocating NULL pointer in IconvLCPTranscoder

[Jerry Napoli]

Hi,

While experimenting with memory managers, I noticed that
IconvLCPTranscoder::transcode() passes a NULL pointer occasionally to
MemoryManager::deallocate().  This is for version 2.8.  It looks like
when transcoding small strings it uses a stack allocated buffer as the
target, and dynamically allocates through the memory manager when it's
larger:

if (maxChars >= gTempBuffArraySize)
         wideCharBuf = allocatedArray = (wchar_t*) manager->allocate
         (
             (maxChars + 1) * sizeof(wchar_t)
         );//new wchar_t[maxChars + 1];
     else
         wideCharBuf = tmpWideCharArr;

But it unconditionally calls 'manager->deallocate(allocatedArray)' at
the end of the function.  I'm guessing it would be best to deallocate it
only if it's non-NULL since some runtime's might complain passing a NULL
into ::delete().

Thanks,
Jerry


---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@...
For additional commands, e-mail: c-dev-help@...

[Boris Kolpackov-2]

Hi Jerry,

Jerry Napoli <jnapoli@...> writes:

> I'm guessing it would be best to deallocate it only if it's non-NULL
> since some runtime's might complain passing a NULL into ::delete().

I've made the fix. Thanks for reporting this!

Boris

--
Boris Kolpackov, Code Synthesis Tools   http://codesynthesis.com/~boris/blog
Open source XML data binding for C++:   http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde

---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@...
For additional commands, e-mail: c-dev-help@...

[David Bertoni]

Jerry Napoli wrote:

> Hi,
>
> While experimenting with memory managers, I noticed that
> IconvLCPTranscoder::transcode() passes a NULL pointer occasionally to
> MemoryManager::deallocate().  This is for version 2.8.  It looks like
> when transcoding small strings it uses a stack allocated buffer as the
> target, and dynamically allocates through the memory manager when it's
> larger:
>
> if (maxChars >= gTempBuffArraySize)
>         wideCharBuf = allocatedArray = (wchar_t*) manager->allocate
>         (
>             (maxChars + 1) * sizeof(wchar_t)
>         );//new wchar_t[maxChars + 1];
>     else
>         wideCharBuf = tmpWideCharArr;
>
> But it unconditionally calls 'manager->deallocate(allocatedArray)' at
> the end of the function.  I'm guessing it would be best to deallocate it
> only if it's non-NULL since some runtime's might complain passing a NULL
> into ::delete().

There are likely more places where NULL pointers end up getting passed
to MemoryManager::deallocate(), so I think we ought to make it a
requirement that deallocate() work correctly with a null pointer value.

Besides, the C++ standard requires that the deallocation functions
accept null pointer values.  Section 3.7.3.2, "Deallocation functions"
states:

"3. The value of the first argument supplied to one of the deallocation
functions provided in the standard library may be a null pointer value;
if so, the call to the deallocation function has no effect. Otherwise,
the value supplied to operator delete(void*) in the standard library
shall be one of the values returned by a previous invocation of either
operator new(size_t) or operator new(size_t, const std::nothrow_t&) in
the standard library, and the value supplied to operator delete[](void*)
in the standard library shall be one of the values returned by a
previous invocation of either operator new[](size_t) or operator
new[](size_t, const std::nothrow_t&) in the standard library."

That said, I think it makes sense for us to clean up any places where we
know that pointer value is null, and doing so doesn't require extra logic.

Dave

---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@...
For additional commands, e-mail: c-dev-help@...

[Boris Kolpackov-2]

Hi David,

David Bertoni <dbertoni@...> writes:

> There are likely more places where NULL pointers end up getting passed
> to MemoryManager::deallocate(), so I think we ought to make it a
> requirement that deallocate() work correctly with a null pointer value.

Ok, I've added the check to deallocate. We, however, should try
to clean up as many places as possible since it is a completely
unnecessary virtual function call.

Boris

--
Boris Kolpackov, Code Synthesis Tools   http://codesynthesis.com/~boris/blog
Open source XML data binding for C++:   http://codesynthesis.com/products/xsd
Mobile/embedded validating XML parsing: http://codesynthesis.com/products/xsde

---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@...
For additional commands, e-mail: c-dev-help@...