diff --git a/Modules/Core/Common/include/itkArray.hxx b/Modules/Core/Common/include/itkArray.hxx index c71ba78b591..ed276453c25 100644 --- a/Modules/Core/Common/include/itkArray.hxx +++ b/Modules/Core/Common/include/itkArray.hxx @@ -54,8 +54,7 @@ template Array::Array(ValueType * datain, SizeValueType sz, bool LetArrayManageMemory) : m_LetArrayManageMemory(LetArrayManageMemory) { - vnl_vector::data = datain; - vnl_vector::num_elmts = sz; + vnl_vector::protected_set_data(datain, sz, LetArrayManageMemory); } #if defined(ITK_LEGACY_REMOVE) @@ -82,7 +81,7 @@ Array::~Array() { if (!m_LetArrayManageMemory) { - vnl_vector::data = nullptr; + vnl_vector::protected_set_data(nullptr, 0, false); } } @@ -94,8 +93,7 @@ Array::SetDataSameSize(TValue * datain, bool LetArrayManageMemory) { vnl_vector::destroy(); } - vnl_vector::data = datain; - // NOTE: Required to have same size vnl_vector< TValue >::num_elmts = sz; + vnl_vector::protected_set_data(datain, this->size(), LetArrayManageMemory); m_LetArrayManageMemory = LetArrayManageMemory; } @@ -107,8 +105,7 @@ Array::SetData(TValue * datain, SizeValueType sz, bool LetArrayManageMem { vnl_vector::destroy(); } - vnl_vector::data = datain; - vnl_vector::num_elmts = sz; + vnl_vector::protected_set_data(datain, sz, LetArrayManageMemory); m_LetArrayManageMemory = LetArrayManageMemory; } @@ -122,7 +119,7 @@ Array::SetSize(SizeValueType sz) // on a resize if (!m_LetArrayManageMemory) { - vnl_vector::data = nullptr; + vnl_vector::protected_set_data(nullptr, 0, true); } // Call the superclass's set_size diff --git a/Modules/Core/Common/test/itkArrayGTest.cxx b/Modules/Core/Common/test/itkArrayGTest.cxx index 239696969f3..6cce0e830b2 100644 --- a/Modules/Core/Common/test/itkArrayGTest.cxx +++ b/Modules/Core/Common/test/itkArrayGTest.cxx @@ -22,6 +22,16 @@ #include #include +#if defined(__has_feature) +# if __has_feature(address_sanitizer) +# include +# 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; + + 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; + + 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; + + 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 +} diff --git a/Modules/ThirdParty/VNL/CMakeLists.txt b/Modules/ThirdParty/VNL/CMakeLists.txt index 49b1670c85f..80acf49ed9b 100644 --- a/Modules/ThirdParty/VNL/CMakeLists.txt +++ b/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 )