itkErodeObjectMorphologyImageFilterTest write/write race with SetPixel()

[seanm]

Running the itkParallelSparseFieldLevelSetImageFilterTest test with TSan:

WARNING: ThreadSanitizer: data race (pid=9174)
  Write of size 2 at 0x000107a039c8 by thread T10:
    #0 itk::NeighborhoodAccessorFunctor<itk::Image<unsigned short, 2u>>::Set(unsigned short*, unsigned short const&) const itkNeighborhoodAccessorFunctor.h:78 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001e7924)
    #1 itk::NeighborhoodIterator<itk::Image<unsigned short, 2u>, itk::ZeroFluxNeumannBoundaryCondition<itk::Image<unsigned short, 2u>, itk::Image<unsigned short, 2u>>>::SetPixel(unsigned int, unsigned short const&, bool&) itkNeighborhoodIterator.hxx:108 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001e760c)
    #2 itk::ErodeObjectMorphologyImageFilter<itk::Image<unsigned short, 2u>, itk::Image<unsigned short, 2u>, itk::BinaryBallStructuringElement<unsigned short, 2u, itk::NeighborhoodAllocator<unsigned short>>>::Evaluate(itk::NeighborhoodIterator<itk::Image<unsigned short, 2u>, itk::ZeroFluxNeumannBoundaryCondition<itk::Image<unsigned short, 2u>, itk::Image<unsigned short, 2u>>>&, itk::BinaryBallStructuringElement<unsigned short, 2u, itk::NeighborhoodAllocator<unsigned short>> const&) itkErodeObjectMorphologyImageFilter.hxx:48 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001dfd84)
    #3 itk::ObjectMorphologyImageFilter<itk::Image<unsigned short, 2u>, itk::Image<unsigned short, 2u>, itk::BinaryBallStructuringElement<unsigned short, 2u, itk::NeighborhoodAllocator<unsigned short>>>::DynamicThreadedGenerateData(itk::ImageRegion<2u> const&) itkObjectMorphologyImageFilter.hxx:159 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001df1c4)
<snip>

  Previous write of size 2 at 0x000107a039c8 by thread T11:
    #0 itk::DefaultPixelAccessor<unsigned short>::Set(unsigned short&, unsigned short const&) const itkDefaultPixelAccessor.h:69 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001300cc)
    #1 itk::DefaultPixelAccessorFunctor<itk::Image<unsigned short, 2u>>::Set(unsigned short&, unsigned short const&) const itkDefaultPixelAccessorFunctor.h:99 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x100130058)
    #2 itk::ImageRegionIterator<itk::Image<unsigned short, 2u>>::Set(unsigned short const&) const itkImageRegionIterator.h:118 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x10012f2d0)
    #3 itk::ObjectMorphologyImageFilter<itk::Image<unsigned short, 2u>, itk::Image<unsigned short, 2u>, itk::BinaryBallStructuringElement<unsigned short, 2u, itk::NeighborhoodAllocator<unsigned short>>>::DynamicThreadedGenerateData(itk::ImageRegion<2u> const&) itkObjectMorphologyImageFilter.hxx:119 (ITKBinaryMathematicalMorphologyTestDriver:arm64+0x1001dee60)
<snip>

We see from the above that one thread is writing to 0x000107a039c8 and another is simultaneously writing to the same address. I didn't check if each thread is writing the same value or not.

Here is the relevant code, NB the fire emojis:

template <typename TInputImage, typename TOutputImage, typename TKernel>
void
ObjectMorphologyImageFilter<TInputImage, TOutputImage, TKernel>::DynamicThreadedGenerateData(
  const OutputImageRegionType & outputRegionForThread)
{
  ImageRegionConstIterator<TInputImage> iRegIter;
  ImageRegionIterator<TOutputImage>     oRegIter;
  iRegIter = ImageRegionConstIterator<InputImageType>(this->GetInput(), outputRegionForThread);
  oRegIter = ImageRegionIterator<OutputImageType>(this->GetOutput(), outputRegionForThread);
  /* Copy the input image to the output image - then only boundary pixels
   * need to be changed in the output image */
  while (!oRegIter.IsAtEnd())
  {
    if (Math::NotExactlyEquals(oRegIter.Get(), m_ObjectValue))
    {
      oRegIter.Set(iRegIter.Get());// 🔥🔥🔥🔥 "Previous write of size 2"
    }
    ++oRegIter;
    ++iRegIter;
  }

  // Find the boundary "faces"
  NeighborhoodAlgorithm::ImageBoundaryFacesCalculator<InputImageType>                        fC;
  typename NeighborhoodAlgorithm::ImageBoundaryFacesCalculator<InputImageType>::FaceListType faceList =
    fC(this->GetInput(), outputRegionForThread, m_Kernel.GetRadius());


  // Setup the kernel that spans the immediate neighbors of the current
  // input pixel - used to determine if that pixel abuts a non-object
  // pixel, i.e., is a boundary pixel
  constexpr auto bKernelSize = MakeFilled<RadiusType>(1);

  TotalProgressReporter progress(this, this->GetOutput()->GetRequestedRegion().GetNumberOfPixels());

  OutputNeighborhoodIteratorType oSNIter;
  InputNeighborhoodIteratorType  iSNIter;
  for (const auto & face : faceList)
  {
    oSNIter = OutputNeighborhoodIteratorType(m_Kernel.GetRadius(), this->GetOutput(), face);
    // No need to overwrite on output...and m_BoundaryCondition is
    // templated over inputImageType - and cannot be applied to the
    // output image
    // oSNIter.OverrideBoundaryCondition(m_BoundaryCondition);
    oSNIter.GoToBegin();

    iSNIter = InputNeighborhoodIteratorType(bKernelSize, this->GetInput(), face);
    iSNIter.OverrideBoundaryCondition(m_BoundaryCondition);
    iSNIter.GoToBegin();

    while (!iSNIter.IsAtEnd())
    {
      if (Math::ExactlyEquals(iSNIter.GetCenterPixel(), m_ObjectValue))
      {
        if (this->IsObjectPixelOnBoundary(iSNIter))
        {
          this->Evaluate(oSNIter, m_Kernel); // 🔥🔥🔥🔥 "Write of size 2", this Evaluate() calls SetPixel()
        }
      }
      ++iSNIter;
      ++oSNIter;
      progress.CompletedPixel();
    }
  }
}

[N-Dekker]

Juts wondering... Could it be that multiple threads share one and the same region as outputRegionForThread? Or at least, could there be any overlap between the output regions of threads?

Otherwise, could it possibly be that input and output image may be one and the same image?

Honestly, I'm clueless 🤷

[blowekamp]

Honestly, I'm clueless 🤷

I see two potential issues. It looks like the neighborhood iterators may go outside the outputRegionForThread.

1. The first loop copies to the output, but the second loop may use pixel outside of thread region pixels as input.
2. I am not sure if the "Evaluate" method writes outside of the thread region.

Edited for sanity.