BUG: Make VariableLengthVector::m_Data null when its length is zero

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`).

Author: N-Dekker

Modules/Core/Common/include/itkVariableLengthVector.h

@@ -270,10 +270,13 @@ class ITK_TEMPLATE_EXPORT VariableLengthVector
     void
     operator()(unsigned int newSize, unsigned int oldSize, TValue2 * oldBuffer, TValue2 * newBuffer) const
     {
-      itkAssertInDebugAndIgnoreInReleaseMacro(newBuffer);
       const size_t nb = std::min(newSize, oldSize);
-      itkAssertInDebugAndIgnoreInReleaseMacro(nb == 0 || (nb > 0 && oldBuffer != nullptr));
-      std::copy_n(oldBuffer, nb, newBuffer);
+      if (nb > 0)
+      {
+        itkAssertInDebugAndIgnoreInReleaseMacro(newBuffer);
+        itkAssertInDebugAndIgnoreInReleaseMacro(oldBuffer);
+        std::copy_n(oldBuffer, nb, newBuffer);
+      }
     }
   };
 

Modules/Core/Common/include/itkVariableLengthVector.hxx

@@ -29,18 +29,15 @@ namespace itk
 
 template <typename TValue>
 VariableLengthVector<TValue>::VariableLengthVector(unsigned int length)
-  : m_Data(new TValue[length])
+  : m_Data(length == 0 ? nullptr : new TValue[length])
   , m_NumElements(length)
-{
-  // postcondition(s)
-  itkAssertInDebugAndIgnoreInReleaseMacro(m_Data != nullptr);
-}
+{}
 
 template <typename TValue>
 VariableLengthVector<TValue>::VariableLengthVector(unsigned int length, const TValue & value)
   : VariableLengthVector(length)
 {
-  std::fill_n(m_Data, length, value);
+  Fill(value);
 }
 
 template <typename TValue>
@@ -67,10 +64,6 @@ VariableLengthVector<TValue>::VariableLengthVector(const VariableLengthVector<TV
     itkAssertInDebugAndIgnoreInReleaseMacro(v.m_Data != nullptr);
     std::copy_n(&v.m_Data[0], m_NumElements, &this->m_Data[0]);
   }
-  else
-  {
-    m_Data = nullptr;
-  }
 }
 
 template <typename TValue>
@@ -133,8 +126,6 @@ VariableLengthVector<TValue>::VariableLengthVector(
     rhs)
   : VariableLengthVector(rhs.Size())
 {
-  // VariableLengthVector(length) post-condition
-  itkAssertInDebugAndIgnoreInReleaseMacro(m_Data != nullptr);
   for (ElementIdentifier i = 0; i < m_NumElements; ++i)
   {
     this->m_Data[i] = static_cast<TValue>(rhs[i]);
@@ -191,11 +182,14 @@ VariableLengthVector<TValue>::Reserve(ElementIdentifier size)
   }
   else
   {
-    m_Data = new TValue[size];
+    // At this point, m_Data is null.
+    if (size > 0)
+    {
+      m_Data = new TValue[size];
+    }
     m_NumElements = size;
     m_LetArrayManageMemory = true;
   }
-  itkAssertInDebugAndIgnoreInReleaseMacro(m_Data != nullptr);
 }
 
 #ifndef ITK_FUTURE_LEGACY_REMOVE
@@ -274,10 +268,15 @@ VariableLengthVector<TValue>::SetSize(unsigned int sz, TReallocatePolicy realloc
 
   if (reallocatePolicy(sz, m_NumElements) || !m_LetArrayManageMemory)
   {
-    auto temp = make_unique_for_overwrite<TValue[]>(sz); // may throw
-    itkAssertInDebugAndIgnoreInReleaseMacro(temp);
-    itkAssertInDebugAndIgnoreInReleaseMacro(m_NumElements == 0 || (m_NumElements > 0 && m_Data != nullptr));
-    keepValues(sz, m_NumElements, m_Data, temp.get());
+    std::unique_ptr<TValue[]> temp;
+
+    if (sz > 0)
+    {
+      temp = make_unique_for_overwrite<TValue[]>(sz); // may throw
+      itkAssertInDebugAndIgnoreInReleaseMacro(temp);
+      itkAssertInDebugAndIgnoreInReleaseMacro(m_NumElements == 0 || (m_NumElements > 0 && m_Data != nullptr));
+      keepValues(sz, m_NumElements, m_Data, temp.get());
+    }
     // commit changes
     if (m_LetArrayManageMemory)
     {
@@ -293,10 +292,11 @@ template <typename TValue>
 void
 VariableLengthVector<TValue>::Fill(const TValue & v)
 {
-  itkAssertInDebugAndIgnoreInReleaseMacro(m_NumElements == 0 || (m_NumElements > 0 && m_Data != nullptr));
-  // VC++ version of std::fill_n() expects the output iterator to be valid
-  // instead of expecting the range [OutIt, OutIt+n) to be valid.
-  std::fill(&this->m_Data[0], &this->m_Data[m_NumElements], v);
+  if (m_NumElements > 0)
+  {
+    itkAssertInDebugAndIgnoreInReleaseMacro(m_Data);
+    std::fill_n(m_Data, m_NumElements, v);
+  }
 }
 
 template <typename TValue>