COMP: Adopt VXL private vnl_vector ivars via protected_set_data

Modules/Core/Common/include/itkArray.hxx

@@ -54,8 +54,7 @@ template <typename TValue>
 Array<TValue>::Array(ValueType * datain, SizeValueType sz, bool LetArrayManageMemory)
   : m_LetArrayManageMemory(LetArrayManageMemory)
 {
-  vnl_vector<TValue>::data = datain;
-  vnl_vector<TValue>::num_elmts = sz;
+  vnl_vector<TValue>::protected_set_data(datain, sz, LetArrayManageMemory);
 }
 
 #if defined(ITK_LEGACY_REMOVE)
@@ -82,7 +81,7 @@ Array<TValue>::~Array()
 {
   if (!m_LetArrayManageMemory)
   {
-    vnl_vector<TValue>::data = nullptr;
+    vnl_vector<TValue>::protected_set_data(nullptr, 0, false);
   }
 }
 
@@ -94,8 +93,7 @@ Array<TValue>::SetDataSameSize(TValue * datain, bool LetArrayManageMemory)
   {
     vnl_vector<TValue>::destroy();
   }
-  vnl_vector<TValue>::data = datain;
-  // NOTE: Required to have same size vnl_vector< TValue >::num_elmts = sz;
+  vnl_vector<TValue>::protected_set_data(datain, this->size(), LetArrayManageMemory);
   m_LetArrayManageMemory = LetArrayManageMemory;
 }
 
@@ -107,8 +105,7 @@ Array<TValue>::SetData(TValue * datain, SizeValueType sz, bool LetArrayManageMem
   {
     vnl_vector<TValue>::destroy();
   }
-  vnl_vector<TValue>::data = datain;
-  vnl_vector<TValue>::num_elmts = sz;
+  vnl_vector<TValue>::protected_set_data(datain, sz, LetArrayManageMemory);
   m_LetArrayManageMemory = LetArrayManageMemory;
 }
 
@@ -122,7 +119,7 @@ Array<TValue>::SetSize(SizeValueType sz)
     // on a resize
     if (!m_LetArrayManageMemory)
     {
-      vnl_vector<TValue>::data = nullptr;
+      vnl_vector<TValue>::protected_set_data(nullptr, 0, true);
     }
 
     // Call the superclass's set_size

Modules/Core/Common/test/itkArrayGTest.cxx

@@ -22,6 +22,16 @@
 #include <gtest/gtest.h>
 #include <iostream>
 
+#if defined(__has_feature)
+#  if __has_feature(address_sanitizer)
+#    include <sanitizer/lsan_interface.h>
+#    define ITK_ARRAY_GTEST_HAS_ASAN 1
+#  endif
+#endif
+#ifndef ITK_ARRAY_GTEST_HAS_ASAN
+#  define ITK_ARRAY_GTEST_HAS_ASAN 0
+#endif
+
 namespace
 {
 // Checks that `itk::Array` supports class template argument deduction (CTAD).
@@ -124,3 +134,141 @@ TEST(Array, MemoryManagement)
 
   delete[] data;
 }
+
+
+// Test 1: Regression for the SetSize-on-non-owning-Array path. The vnl_vector
+// allocation macro asserts that vnl's m_LetArrayManageMemory flag is true
+// before allocating; if Array::SetSize hands a stale `false` to vnl via
+// protected_set_data, this aborts in debug builds and silently leaks in
+// release. Exercises the path on a non-owning Array and on a freshly
+// default-constructed (size-zero) Array.
+TEST(Array, SetSizeOnNonOwningArrayDoesNotAssertOrLeak)
+{
+  using FloatArrayType = itk::Array<float>;
+
+  constexpr unsigned int n{ 7 };
+  float                  buffer[n]{};
+
+  FloatArrayType external;
+  external.SetData(buffer, n, /*LetArrayManageMemory=*/false);
+
+  external.SetSize(n + 1);
+  external.Fill(3.0f);
+  EXPECT_EQ(external.GetSize(), n + 1u);
+
+  FloatArrayType empty;
+  empty.SetSize(5);
+  empty.Fill(2.0f);
+  EXPECT_EQ(empty.GetSize(), 5u);
+}
+
+
+// Test 2: Cycle through every ownership transition that protected_set_data
+// gates. Each step exercises a different combination of (old ITK flag, new
+// caller intent). Under AddressSanitizer + LeakSanitizer the dtor chain
+// reports any double-free or leak left behind by a wrong third argument.
+TEST(Array, OwnershipTransitionsRoundTripCleanly)
+{
+  using FloatArrayType = itk::Array<float>;
+
+  constexpr unsigned int n{ 5 };
+
+  // Externally-owned buffers used across the transitions.
+  float external_a[n]{};
+  float external_b[n]{};
+
+  // Heap buffer the Array will be told to manage and free on destruction.
+  auto * heap_owned = new float[n]{};
+
+  // Path: owning -> external (false). Old ITK flag was true, so SetData
+  // calls vnl::destroy() before re-pointing.
+  FloatArrayType a(n);
+  a.Fill(1.0f);
+  a.SetData(external_a, n, /*LetArrayManageMemory=*/false);
+  EXPECT_EQ(a.GetSize(), n);
+
+  // Path: external (false) -> external (false). No allocation either side.
+  a.SetData(external_b, n, /*LetArrayManageMemory=*/false);
+
+  // Path: external (false) -> heap-owned (true). Caller hands ownership
+  // of `heap_owned` to the Array; Array dtor must free it via vnl.
+  a.SetData(heap_owned, n, /*LetArrayManageMemory=*/true);
+
+  // SetDataSameSize path with the SAME flag transitions.
+  FloatArrayType b(n);
+  b.Fill(1.0f);
+  b.SetDataSameSize(external_a, /*LetArrayManageMemory=*/false);
+  b.SetDataSameSize(external_b, /*LetArrayManageMemory=*/false);
+
+  // Constructor with externally-owned + non-owning destruction.
+  {
+    FloatArrayType ext_ctor(external_a, n, /*LetArrayManageMemory=*/false);
+    EXPECT_EQ(ext_ctor.GetSize(), n);
+  } // dtor runs; external_a must NOT be freed.
+
+  // Constructor with heap buffer + owning destruction.
+  {
+    auto *         heap2 = new float[n]{};
+    FloatArrayType heap_ctor(heap2, n, /*LetArrayManageMemory=*/true);
+    EXPECT_EQ(heap_ctor.GetSize(), n);
+  } // dtor must free heap2 via vnl_c_vector::deallocate.
+
+  // `a` still owns `heap_owned`; let its dtor free it here.
+  // (No explicit delete[] — that would double-free under the fix.)
+}
+
+
+// Test 3: Heap-buffer ownership reaches a real free. Without a numeric T
+// that can hook ITK's allocator, leak detection comes from ASan/LSan; in a
+// non-sanitizer build the test still exercises the path and verifies that
+// the operations complete without crashing or asserting. The
+// __lsan_do_recoverable_leak_check call (only compiled in under ASan)
+// reports any leftover allocations from this scope as a test failure.
+TEST(Array, HeapBufferOwnedByArrayIsFreed)
+{
+  using FloatArrayType = itk::Array<float>;
+
+  constexpr unsigned int n{ 11 };
+
+  // Constructor path.
+  {
+    auto *         heap = new float[n]{};
+    FloatArrayType owner(heap, n, /*LetArrayManageMemory=*/true);
+    owner.Fill(7.0f);
+    EXPECT_EQ(owner.GetSize(), n);
+  }
+
+  // SetData(true) path on a previously-owning Array.
+  {
+    FloatArrayType owner(n);
+    owner.Fill(2.0f);
+    auto * heap = new float[n]{};
+    owner.SetData(heap, n, /*LetArrayManageMemory=*/true);
+    EXPECT_EQ(owner.GetSize(), n);
+  }
+
+  // SetDataSameSize(true) path on a previously-owning Array.
+  {
+    FloatArrayType owner(n);
+    owner.Fill(3.0f);
+    auto * heap = new float[n]{};
+    owner.SetDataSameSize(heap, /*LetArrayManageMemory=*/true);
+    EXPECT_EQ(owner.GetSize(), n);
+  }
+
+  // SetSize-on-external path: the Array must take ownership of the
+  // freshly-allocated buffer that vnl_vector::set_size produces.
+  {
+    float          buf[n]{};
+    FloatArrayType external;
+    external.SetSize(n);
+    external.SetData(buf, n, /*LetArrayManageMemory=*/false);
+    external.SetSize(n + 3); // allocates a new buffer; Array now owns it.
+    external.Fill(4.0f);
+    EXPECT_EQ(external.GetSize(), n + 3u);
+  }
+
+#if ITK_ARRAY_GTEST_HAS_ASAN
+  __lsan_do_recoverable_leak_check();
+#endif
+}

Modules/ThirdParty/VNL/CMakeLists.txt

@@ -5,10 +5,7 @@ option(ITK_USE_SYSTEM_VXL "Use an outside build of VXL." OFF)
 mark_as_advanced(ITK_USE_SYSTEM_VXL)
 
 if(ITK_USE_SYSTEM_VXL)
-  find_package(VXL 3.0.0 REQUIRED)
-  # 3.0.0 is the first version of VXL that supports
-  # the requirement of front() and back() members
-  # in the vnl_vector and vnl_vector_fixed.
+  find_package(VXL 4.0.0 REQUIRED)
   set(
     ITKVNL_SYSTEM_INCLUDE_DIRS
     ${VXL_NETLIB_INCLUDE_DIR}
@@ -103,27 +100,16 @@ else()
   # set(VXL_USE_DCMTK                    OFF CACHE BOOL "VXL configuration for ITK" FORCE) # <- Not visible in itk extracted code
   # set(VXL_USE_ECW                      OFF CACHE BOOL "VXL configuration for ITK" FORCE) # <- Visible, but unreachable in itk extracted code
   set(VXL_USE_GEOTIFF OFF CACHE BOOL "VXL configuration for ITK" FORCE)
-  if(ITK_LEGACY_REMOVE OR ITK_FUTURE_LEGACY_REMOVE)
-    set(
-      VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS
-      OFF
-      CACHE BOOL
-      "Allow default deprecated implicit conversions."
-      FORCE
-    )
-  else()
-    set(
-      VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS
-      ON
-      CACHE BOOL
-      "Allow default deprecated implicit conversions."
-      FORCE
-    )
-  endif()
-  # MUST ALWAYS BE ON currently
+  set(
+    VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS
+    OFF
+    CACHE BOOL
+    "Allow default deprecated implicit conversions."
+    FORCE
+  )
   set(
     VXL_USE_HISTORICAL_PROTECTED_IVARS
-    ON
+    OFF
     CACHE BOOL
     "VXL configuration for ITK"
     FORCE
@@ -205,8 +191,8 @@ mark_as_advanced(
   #VXL_USE_DCMTK <- Not visible in itk extracted code
   #VXL_USE_ECW   <- Visible, but unreachable in itk extracted code
   VXL_USE_GEOTIFF # <- Visible, and must be off
-  VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS # <- must track ITK LegacyRemove setting
-  VXL_USE_HISTORICAL_PROTECTED_IVARS # <- must track ITK LegacyRemove setting
+  VXL_USE_HISTORICAL_IMPLICIT_CONVERSIONS # <- forced OFF; ITK targets the modern VNL API
+  VXL_USE_HISTORICAL_PROTECTED_IVARS # <- forced OFF; ITK targets the modern VNL API
   VXL_USE_LFS # <- Visible, and must be off
   #VXL_VIL_INCLUDE_IMAGE_IO <- Not visible in itk extracted code
 )