BUG: Make VariableLengthVector::m_Data null when its length is zero#5713
Conversation
Originally, a zero-length VariableLengthVector might still have its `m_Data` pointing to the heap, as it might have its memory allocated by `new TValue[length]`, with `length == 0`. A memory leak in `VariableLengthVector(unsigned int length)` was then introduced by pull request InsightSoftwareConsortium#5710 commit bb79801 "STYLE: Replace AllocateElements calls within VariableLengthVector" forgetting to `delete` memory that was allocated by `new TValue[0]`. As was reported by Mihail Isakov, referring to the Valgrind results at https://open.cdash.org/builds/10917349/dynamic_analysis With this commit, m_Data should always be null when the VariableLengthVector is empty and manages its own memory (m_LetArrayManageMemory is `true`).
github-actions
bot
added
type:Bug
| // VC++ version of std::fill_n() expects the output iterator to be valid | ||
| // instead of expecting the range [OutIt, OutIt+n) to be valid. |
There was a problem hiding this comment.
Removed this comment, as it does not apply to currently supported versions of VC++ anymore, as far as I can see.
David Cole (@dlrdave) mentioned in https://review.source.kitware.com/%23/c/20307/, Oct 24, 2015:
You cannot call std::fill_n with a nullptr for Debug VS 2013.
The implementation of fill_n looks like this:
template<class _OutIt,
class _Diff,
class _Ty> inline
_OutIt fill_n(_OutIt _Dest, _Diff _Count, const _Ty& _Val)
{ // copy _Val _Count times through [_Dest, ...)
_DEBUG_POINTER(_Dest);
return (_Fill_n(_Dest, _Count, _Val,
_Is_checked(_Dest)));
}
The _DEBUG_POINTER line is called unconditionally, and will throw this assert if the pointer is nullptr.
However, this appears obsolete now, as VS 2019 and VS2022 no longer have those _DEBUG_POINTER macro calls. I don't see them in https://github.com/microsoft/STL/ either.
Honestly I don't know if the C++ standard now officially allows passing a null pointer as iterator to fill_n, or any other STL algorithm. 🤷
|
macOS-Build20656084594 test itkFFTConvolutionImageFilterStreamingTest fails, saying: I guess it's unrelated to this pull request 🤷 |
|
It looks like "itkFFTConvolutionImageFilterStreamingTestOutput.mha" is used for multiple outputs in the tests in the CMake file. |
|
Addressing the duplicate output file race condition in #5714 |
dzenanz
merged commit b15f46a
into
InsightSoftwareConsortium:main
4 participants
Originally, a zero-length VariableLengthVector might still have its
m_Datapointing to the heap, as it might have its memory allocated bynew TValue[length], withlength == 0.A memory leak in
VariableLengthVector(unsigned int length)was then introduced by pull request #5710 commit bb79801"STYLE: Replace AllocateElements calls within VariableLengthVector" forgetting to
deletememory that was allocated bynew TValue[0]. As was reported by Mihail Isakov (@issakomi) at #5710 (comment), referring to the Valgrind results at https://open.cdash.org/builds/10917349/dynamic_analysisWith this commit, m_Data should always be null when the VariableLengthVector is empty and manages its own memory (m_LetArrayManageMemory is
true).