Undefined behavior of iterators Get for VariableLengthVector

[SimonRit]

Description

The Get function of an iterator on an itk::Image of itk::VariableLengthVector returns an itk::VariableLengthVector which data points to a pixel of the itk::Image. This is because the DefaultVectorPixelAccessor::Get calls the VariableLengthVector constructor parameterized by a const pointer. This leads to an unexpected behavior for the user.

Steps to Reproduce

This is a minimal example reproducing the issue:

#include <itkVectorImage.h>
#include <itkImageRegionIterator.h>

int
main(int argc, char * argv[])
{
  using ImageType = itk::VectorImage<float,2>;
  auto img = ImageType::New();
  img->SetVectorLength(3);
  ImageType::SizeType size = { { 2, 2 } };
  img->SetRegions(size);
  img->AllocateInitialized();
  itk::ImageRegionIterator<ImageType> it(img, img->GetLargestPossibleRegion());
  auto v { it.Get() };
  float c = 0.;
  while(!it.IsAtEnd())
  {
    v.Fill(c++);
    it.Set(v);
    ++it;
  }
  it.GoToBegin();
  while(!it.IsAtEnd())
  {
    std::cout << it.Get() << std::endl;
    ++it;
  }
  return EXIT_SUCCESS;
}

Expected behavior

It should display

[0, 0, 0]
[1, 1, 1]
[2, 2, 2]
[3, 3, 3]

Actual behavior

It actually displays

[3, 3, 3]
[1, 1, 1]
[2, 2, 2]
[3, 3, 3]

Reproducibility

100%

Versions

All versions since 6a60327.

Environment

All OSs.

Additional Information

[blowekamp]

I am uncertain if this setting with the ImageRegionIterator Get returned value is fully a bug, or just using a "reference" as an aggressive optimization.

However, the following with an ImageRegionConstIterator is also modifying the buffer:

 itk::ImageRegionConstIterator<ImageType> constIt(img, img->GetLargestPossibleRegion());
  auto                                  v{ constIt.Get() };
  
  itk::ImageRegionIterator<ImageType> it(img, img->GetLargestPossibleRegion());
  float                               c = 0.;
  while (!it.IsAtEnd())
  {
    v.Fill(c++);
    it.Set(v);
    ++it;
  }

And that is clearly non-const correct behavior. I am wondering if the returned type for Get is correct for the VectorImages, perhaps is should be const?

Here is an AI agent summary of the current Get methods for iterators:

`itkImageConstIterator.h` - Returns PixelTye - uses `m_PixelAccessorFunctor.Get()`
`itkConditionalConstIterator.h` - Returns const PixelType (pure virtual)
`itkLineConstIterator.h` - Returns const PixelType - uses `m_Image->GetPixel()`
`itkConstShapedNeighborhoodIterator.h` - Returns PixelType - uses `GetPixel()`
`itkImageConstIteratorWithIndex.h` - Returns PixelType - uses `m_PixelAccessorFunctor.Get()`
`itkImageReverseConstIterator.h` - Returns const PixelType - uses `m_PixelAccessorFunctor.Get()`
`itkFloodFilledImageFunctionConditionalIterator.h` - Returns const PixelType (override) - uses `GetPixel()`
`itkShapedFloodFilledFunctionConditionalConstIterator.h` - Returns const PixelType (override) - uses `GetPixel()`
`itkFloodFilledSpatialFunctionConditionalIterator.h` - Returns const PixelType (override) - uses `GetPixel()`
`itkShapedFloodFilledImageFunctionConditionalIterator.h` - Returns const PixelType (override) - uses `GetPixel()`
`itkFloodFilledFunctionConditionalConstIterator.h` - Returns const PixelType (override) - uses `GetPixel()`

If the return type was const PixelType, then the above code would cause a deep copy.

[SimonRit]

It looks to me that there is a Value() method for having a reference to the value. The documentation clearly indicates that you can't use adaptors then. The value is const / should not be modified according to the adaptor API (see Get here).

If making the returned type const PixelType solves the problem at no cost, it sounds like the solution to this bug.