diff --git a/Modules/Core/Common/include/itkDefaultVectorPixelAccessor.h b/Modules/Core/Common/include/itkDefaultVectorPixelAccessor.h index 03db79d72b9..6d82cad7637 100644 --- a/Modules/Core/Common/include/itkDefaultVectorPixelAccessor.h +++ b/Modules/Core/Common/include/itkDefaultVectorPixelAccessor.h @@ -21,6 +21,7 @@ #include "itkMacro.h" #include "itkVariableLengthVector.h" #include "itkIntTypes.h" +#include // for std::min namespace itk { @@ -61,13 +62,30 @@ class ITK_TEMPLATE_EXPORT DefaultVectorPixelAccessor /** Internal type alias. It defines the internal real representation of data. */ using InternalType = TType; - /** Set output using the value in input */ + /** Set output using the value in input. + * + * Copies up to `m_VectorLength` components from `input` into the pixel + * slot at `output` + offset. When `input` is shorter than + * `m_VectorLength`, only the available components are copied; the + * remaining destination components are left unchanged. This guards + * against callers that construct an under-sized source vector — in + * particular, `VariableLengthVector(0)` whose internal data + * pointer is null (see ITK commit 1d87efa5), which previously caused + * a SIGSEGV when the implicit copy dereferenced the null buffer. */ inline void Set(InternalType & output, const ExternalType & input, const SizeValueType offset) const { InternalType * truePixel = (&output) + offset * m_OffsetMultiplier; - for (VectorLengthType i = 0; i < m_VectorLength; ++i) + // A well-formed caller passes a source of length 0 (sentinel / no-op) + // or exactly m_VectorLength. Anything else is a caller bug that was + // silently masked by the pre-1d87efa5 allocation behavior; catch it + // in debug builds. + itkAssertInDebugAndIgnoreInReleaseMacro(input.GetSize() == 0 || input.GetSize() == m_VectorLength); + + const VectorLengthType copyLength = std::min(m_VectorLength, input.GetSize()); + + for (VectorLengthType i = 0; i < copyLength; ++i) { truePixel[i] = input[i]; } diff --git a/Modules/Core/Common/test/itkVariableLengthVectorGTest.cxx b/Modules/Core/Common/test/itkVariableLengthVectorGTest.cxx index fba4b2aff2b..0c58b65cfbf 100644 --- a/Modules/Core/Common/test/itkVariableLengthVectorGTest.cxx +++ b/Modules/Core/Common/test/itkVariableLengthVectorGTest.cxx @@ -19,6 +19,8 @@ // First include the header file to be tested: #include "itkVariableLengthVector.h" #include "itkRangeGTestUtilities.h" +#include "itkVectorImage.h" +#include "itkImageRegionIterator.h" #include #include // For iota. #include // For mt19937. @@ -162,3 +164,94 @@ TEST(VariableLengthVector, ConstructWithSpecifiedLengthAndValue) } } } + + +// Regression test: ensure that setting a VectorImage pixel from a +// VariableLengthVector that is shorter than the image's +// NumberOfComponentsPerPixel does not crash. Before commit +// 1d87efa529, `VariableLengthVector(length)` with length=0 allocated a +// valid (zero-size) heap block whose address was non-null. That commit +// changed the ctor to store `nullptr` for length=0 and caused +// DefaultVectorPixelAccessor::Set — which copies m_VectorLength +// elements from the source VLV regardless of the source's own size — +// to dereference the source's null data pointer, producing SIGSEGV. +// This test documents that a length-mismatched source VLV is safe to +// pass; it is permitted for the target pixel to receive +// implementation-defined values, but the call must not crash. +TEST(VariableLengthVector, VectorImageSetPixelFromShorterSourceIsSafe) +{ + using ValueType = float; + using ImageType = itk::VectorImage; + using PixelType = ImageType::PixelType; + + constexpr unsigned int componentsPerPixel = 3; + auto image = ImageType::New(); + const ImageType::SizeType size = { { 2, 2, 2 } }; + const ImageType::RegionType region{ size }; + image->SetRegions(region); + image->SetNumberOfComponentsPerPixel(componentsPerPixel); + image->Allocate(); + + // Seed every pixel with a known sentinel value so we can assert the + // fix's "trailing components left unchanged" guarantee. + constexpr ValueType sentinelValue = 42.0f; + PixelType sentinel(componentsPerPixel); + sentinel.Fill(sentinelValue); + for (itk::ImageRegionIterator seedIt(image, region); !seedIt.IsAtEnd(); ++seedIt) + { + seedIt.Set(sentinel); + } + + // Pre-regression, VLV(0) had m_Data = new ValueType[0] (non-null). + // Post-regression, m_Data is nullptr. The iterator-based Set() + // path must tolerate that without dereferencing the null buffer. + PixelType shorter(0); + EXPECT_EQ(shorter.GetSize(), 0u); + + itk::ImageRegionIterator it(image, region); + it.GoToBegin(); + ASSERT_FALSE(it.IsAtEnd()); + // The regression manifests here: before the fix, this produces + // a SIGSEGV due to memcpy/load from a null source buffer. + ASSERT_NO_FATAL_FAILURE(it.Set(shorter)); + + // Verify the fix's guarantee: a length-0 source is a no-op; the + // destination pixel retains its prior (sentinel) value. + const auto unchanged = it.Get(); + ASSERT_EQ(unchanged.GetSize(), componentsPerPixel); + EXPECT_FLOAT_EQ(unchanged[0], sentinelValue); + EXPECT_FLOAT_EQ(unchanged[1], sentinelValue); + EXPECT_FLOAT_EQ(unchanged[2], sentinelValue); +} + + +// Companion: verify the normal, length-matched path still works. +TEST(VariableLengthVector, VectorImageSetPixelFromMatchedSourceWorks) +{ + using ValueType = float; + using ImageType = itk::VectorImage; + using PixelType = ImageType::PixelType; + + constexpr unsigned int componentsPerPixel = 3; + auto image = ImageType::New(); + const ImageType::SizeType size = { { 2, 2, 2 } }; + const ImageType::RegionType region{ size }; + image->SetRegions(region); + image->SetNumberOfComponentsPerPixel(componentsPerPixel); + image->Allocate(); + + PixelType matched(componentsPerPixel); + matched[0] = 1.0f; + matched[1] = 2.0f; + matched[2] = 3.0f; + + itk::ImageRegionIterator it(image, region); + it.GoToBegin(); + it.Set(matched); + + const auto roundTrip = it.Get(); + ASSERT_EQ(roundTrip.GetSize(), componentsPerPixel); + EXPECT_FLOAT_EQ(roundTrip[0], 1.0f); + EXPECT_FLOAT_EQ(roundTrip[1], 2.0f); + EXPECT_FLOAT_EQ(roundTrip[2], 3.0f); +}