From 0aef90cbf6bb2e7fc7b8d0518b632383c4ae75e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Tue, 21 Feb 2023 18:23:30 -0500 Subject: [PATCH 1/6] COMP: Fix itkParallelSparseFieldLevelSetImageFilter uninitialized memory Fix uninitialized memory warning in `itk::ParallelSparseFieldLevelSetImageFilter`: add an in-class `{}` default member initializer to its non-static `m_Pad1` and `m_Pad2` data members. Fixes: ``` UMC ==17034== Conditional jump or move depends on uninitialised value(s) ==17034== at 0x483EF49: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==17034== by 0x6561BCD: std::basic_ostream >& std::operator<< >(std::basic_ostream >&, char const*) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28) ==17034== by 0x3B867E: itk::ParallelSparseFieldCityBlockNeighborList, itk::ZeroFluxNeumannBoundaryCondition, itk::Image > > >::Print(std::ostream&, itk::Indent) const (itkParallelSparseFieldLevelSetImageFilter.hxx:86) ==17034== by 0x3B49D9: itk::ParallelSparseFieldLevelSetImageFilter, itk::Image >::PrintSelf(std::ostream&, itk::Indent) const (itkParallelSparseFieldLevelSetImageFilter.hxx:2516) ==17034== by 0x5429E53: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:119) ==17034== by 0x3B1D3F: itk::SmartPointer::Print(std::ostream&) const (itkSmartPointer.h:172) ==17034== by 0x3AF11D: std::ostream& itk::operator<< (std::ostream&, itk::SmartPointer) (itkSmartPointer.h:297) ==17034== by 0x3AD29E: itkParallelSparseFieldLevelSetImageFilterTest(int, char**) (itkParallelSparseFieldLevelSetImageFilterTest.cxx:318) ==17034== by 0x211CA7: main (ITKLevelSetsTestDriver.cxx:272) ``` Raised for example at: https://open.cdash.org/viewDynamicAnalysisFile.php?id=10071614 Related to commit 5e2c49f9eb80e78903ce8f9fd2b5952062653cab. Uncovered by commit c47ed1c1b3e26051de655f7ad03e975fae93b73b. --- .../include/itkParallelSparseFieldLevelSetImageFilter.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h index 17748d6d397..188cae9dc52 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h @@ -124,7 +124,7 @@ class ITK_TEMPLATE_EXPORT ParallelSparseFieldCityBlockNeighborList Print(std::ostream & os, Indent indent) const; private: - char m_Pad1[128]; + char m_Pad1[128]{}; unsigned int m_Size; RadiusType m_Radius; std::vector m_ArrayIndex; @@ -133,7 +133,7 @@ class ITK_TEMPLATE_EXPORT ParallelSparseFieldCityBlockNeighborList /** An internal table for keeping track of stride lengths in a neighborhood, * i.e. the memory offsets between pixels along each dimensional axis. */ unsigned int m_StrideTable[Dimension]; - char m_Pad2[128]; + char m_Pad2[128]{}; }; /** From a14a4958b7a461d619c29bcce22225c1e801b140 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Thu, 23 Feb 2023 22:00:22 -0500 Subject: [PATCH 2/6] COMP: Fix `itk::Histogram` uninitialized memory Fix uninitialized memory warning in `itk::Histogram`: `m_TempMeasurementVector` is an `itk::Array` so rely on its prining capabilities without using the `itk::NumericTraits<>::PrintType` casting. Fixes: ``` UMC ==17753== Conditional jump or move depends on uninitialised value(s) ==17753== at 0x6ED4867: double_conversion::DoubleToStringConverter::ToShortestIeeeNumber(double, double_conversion::StringBuilder*, double_conversion::DoubleToStringConverter::DtoaMode) const (double-to-string.cc:172) ==17753== by 0x55A1556: double_conversion::DoubleToStringConverter::ToShortest(double, double_conversion::StringBuilder*) const (double-to-string.h:216) ==17753== by 0x55A0C60: (anonymous namespace)::ConvertToShortest(double_conversion::DoubleToStringConverter const&, double, double_conversion::StringBuilder&) (itkNumberToString.cxx:31) ==17753== by 0x55A0DB5: std::__cxx11::basic_string, std::allocator > (anonymous namespace)::FloatingPointNumberToString(double) (itkNumberToString.cxx:55) ==17753== by 0x55A0CD4: itk::NumberToString::operator()[abi:cxx11](double) const (itkNumberToString.cxx:71) ==17753== by 0x55A0B3A: std::__cxx11::basic_string, std::allocator > itk::ConvertNumberToString(double) (itkNumberToString.h:86) ==17753== by 0x55A0507: std::ostream& itk::operator<< (std::ostream&, itk::Array const&) (itkArrayOutputSpecialization.cxx:34) ==17753== by 0x63FC43: itk::Statistics::Histogram::PrintSelf(std::ostream&, itk::Indent) const (itkHistogram.hxx:705) ==17753== by 0x555EE53: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:119) ==17753== by 0x66F568: itk::CompareHistogramImageToImageMetric, itk::Image >::PrintSelf(std::ostream&, itk::Indent) const (itkCompareHistogramImageToImageMetric.hxx:158) ==17753== by 0x66DB44: itk::KullbackLeiblerCompareHistogramImageToImageMetric, itk::Image >::PrintSelf(std::ostream&, itk::Indent) const (itkKullbackLeiblerCompareHistogramImageToImageMetric.hxx:99) ==17753== by 0x555EE53: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:119) ==17753== by 0x663AF1: itkKullbackLeiblerCompareHistogramImageToImageMetricTest(int, char**) (itkKullbackLeiblerCompareHistogramImageToImageMetricTest.cxx:303) ==17753== by 0x2CF347: main (ITKRegistrationCommonTestDriver.cxx:417) ``` and ``` UMC ==19839== Conditional jump or move depends on uninitialised value(s) ==19839== at 0x6C8D867: double_conversion::DoubleToStringConverter::ToShortestIeeeNumber(double, double_conversion::StringBuilder*, double_conversion::DoubleToStringConverter::DtoaMode) const (double-to-string.cc:172) ==19839== by 0x54CA58F: double_conversion::DoubleToStringConverter::ToShortestSingle(float, double_conversion::StringBuilder*) const (double-to-string.h:221) ==19839== by 0x54C9C95: (anonymous namespace)::ConvertToShortest(double_conversion::DoubleToStringConverter const&, float, double_conversion::StringBuilder&) (itkNumberToString.cxx:40) ==19839== by 0x54CA0DE: std::__cxx11::basic_string, std::allocator > (anonymous namespace)::FloatingPointNumberToString(float) (itkNumberToString.cxx:55) ==19839== by 0x54C9D2A: itk::NumberToString::operator()[abi:cxx11](float) const (itkNumberToString.cxx:78) ==19839== by 0x54C9BB8: std::__cxx11::basic_string, std::allocator > itk::ConvertNumberToString(float) (itkNumberToString.h:86) ==19839== by 0x54C96DD: std::ostream& itk::operator<< (std::ostream&, itk::Array const&) (itkArrayOutputSpecialization.cxx:54) ==19839== by 0x76AFD3: itk::Statistics::Histogram::PrintSelf(std::ostream&, itk::Indent) const (itkHistogram.hxx:705) ==19839== by 0x5487E53: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:119) ==19839== by 0x7257CF: itk::SmartPointer const>::Print(std::ostream&) const (itkSmartPointer.h:172) ==19839== by 0x711956: std::ostream& itk::operator<< const>(std::ostream&, itk::SmartPointer const>) (itkSmartPointer.h:297) ==19839== by 0x711173: void PrintHistogramInfo const> >(itk::SmartPointer const>) (itkHistogramMatchingImageFilterTest.cxx:126) ==19839== by 0x6F62DB: int itkHistogramMatchingImageFilterTest() (itkHistogramMatchingImageFilterTest.cxx:272) ==19839== by 0x6F2B1D: itkHistogramMatchingImageFilterTest(int, char**) (itkHistogramMatchingImageFilterTest.cxx:412) ==19839== by 0x4735D7: main (ITKImageIntensityTestDriver.cxx:532) ``` Raised for example at: https://open.cdash.org/viewDynamicAnalysisFile.php?id=10071615 https://open.cdash.org/viewDynamicAnalysisFile.php?id=10071616 Uncovered by commit c47ed1c1b3e26051de655f7ad03e975fae93b73b. --- Modules/Numerics/Statistics/include/itkHistogram.hxx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Modules/Numerics/Statistics/include/itkHistogram.hxx b/Modules/Numerics/Statistics/include/itkHistogram.hxx index 625bebc0632..c2597f688cc 100644 --- a/Modules/Numerics/Statistics/include/itkHistogram.hxx +++ b/Modules/Numerics/Statistics/include/itkHistogram.hxx @@ -196,7 +196,10 @@ Histogram::Initialize(const SizeType & size) // initialize auxiliary variables this->m_TempIndex.SetSize(this->GetMeasurementVectorSize()); + m_TempIndex.Fill(0); + this->m_TempMeasurementVector.SetSize(this->GetMeasurementVectorSize()); + m_TempMeasurementVector.Fill(0); // initialize the frequency container m_FrequencyContainer->Initialize(this->m_OffsetTable[this->GetMeasurementVectorSize()]); From cbbbe49bf6d2f6bfd365a147eb55cf04ea0ffed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Tue, 21 Feb 2023 18:43:34 -0500 Subject: [PATCH 3/6] STYLE: Remove an unnecessary static_cast from Histogram::PrintSelf Remove an unnecessary `static_cast` from `itk::Histogram::PrintSelf`: `m_TempMeasurementVector` is an `itk::Array` so rely on its prining capabilities without using the `itk::NumericTraits<>::PrintType` casting. --- Modules/Numerics/Statistics/include/itkHistogram.hxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/Numerics/Statistics/include/itkHistogram.hxx b/Modules/Numerics/Statistics/include/itkHistogram.hxx index c2597f688cc..8d190612ca4 100644 --- a/Modules/Numerics/Statistics/include/itkHistogram.hxx +++ b/Modules/Numerics/Statistics/include/itkHistogram.hxx @@ -704,8 +704,7 @@ Histogram::PrintSelf(std::ostream & os, Inden } } - os << indent << "TempMeasurementVector: " - << static_cast::PrintType>(m_TempMeasurementVector) << std::endl; + os << indent << "TempMeasurementVector: " << m_TempMeasurementVector << std::endl; os << indent << "TempIndex: " << static_cast::PrintType>(m_TempIndex) << std::endl; os << indent << "ClipBinsAtEnds: " << (m_ClipBinsAtEnds ? "On" : "Off") << std::endl; } From d741ad0f54b7724569be8133a20b2a3f53272a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Thu, 23 Feb 2023 21:28:27 -0500 Subject: [PATCH 4/6] STYLE: Prefer in-class `{}` member initializers Prefer using in-class `{}` member initializers in `itk::ParallelSparseFieldLevelSetImageFilter` for its non-static data members. Related to commit 5e2c49f9eb80e78903ce8f9fd2b5952062653cab. --- .../itkParallelSparseFieldLevelSetImageFilter.h | 10 +++++----- .../itkParallelSparseFieldLevelSetImageFilter.hxx | 1 - 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h index 188cae9dc52..9e5bcffd850 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h @@ -125,14 +125,14 @@ class ITK_TEMPLATE_EXPORT ParallelSparseFieldCityBlockNeighborList private: char m_Pad1[128]{}; - unsigned int m_Size; - RadiusType m_Radius; - std::vector m_ArrayIndex; - std::vector m_NeighborhoodOffset; + unsigned int m_Size{ 2 * Dimension }; + RadiusType m_Radius{}; + std::vector m_ArrayIndex{}; + std::vector m_NeighborhoodOffset{}; /** An internal table for keeping track of stride lengths in a neighborhood, * i.e. the memory offsets between pixels along each dimensional axis. */ - unsigned int m_StrideTable[Dimension]; + unsigned int m_StrideTable[Dimension]{}; char m_Pad2[128]{}; }; diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx index 4f44d0a4a08..180fea481ba 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx @@ -49,7 +49,6 @@ ParallelSparseFieldCityBlockNeighborList::ParallelSparseField NeighborhoodType it(m_Radius, dummy_image, dummy_image->GetRequestedRegion()); nCenter = it.Size() / 2; - m_Size = 2 * Dimension; m_ArrayIndex.reserve(m_Size); m_NeighborhoodOffset.reserve(m_Size); From 05c7c5a817896aa89418a3a00a1d81c18a3af666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Thu, 23 Feb 2023 21:35:17 -0500 Subject: [PATCH 5/6] STYLE: Make `PrintSelf` implementation style consistent Make `PrintSelf`implementation style consistent following the ITK SWG coding style guideline: - Use the `os << indent << "{ivarName}: " << m_ivarName << std::endl` recipe: do not print the leading `m_` of the ivar. Following commit c47ed1c1b3e26051de655f7ad03e975fae93b73b. --- .../include/itkParallelSparseFieldLevelSetImageFilter.hxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx index 180fea481ba..919d95258c2 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx @@ -82,7 +82,7 @@ ParallelSparseFieldCityBlockNeighborList::Print(std::ostream os << "ParallelSparseFieldCityBlockNeighborList: " << std::endl; - os << indent << "m_Pad1: " << m_Pad1 << std::endl; + os << indent << "Pad1: " << m_Pad1 << std::endl; os << indent << "Size: " << m_Size << std::endl; os << indent << "Radius: " << m_Radius << std::endl; os << indent << "ArrayIndex: " << m_ArrayIndex << std::endl; From 53d8dbe8e2c23d2b786341893fe522f8e1bd538e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Thu, 23 Feb 2023 21:58:09 -0500 Subject: [PATCH 6/6] STYLE: Remove duplicate statement in `itk::Histogram::Initialize` Remove duplicate statement in `itk::Histogram::Initialize`: https://github.com/InsightSoftwareConsortium/ITK/blob/f0d342e0b81e690194cd3fe993316bca7570402b/Modules/Numerics/Statistics/include/itkHistogram.hxx#L179 and https://github.com/InsightSoftwareConsortium/ITK/blob/f0d342e0b81e690194cd3fe993316bca7570402b/Modules/Numerics/Statistics/include/itkHistogram.hxx#L198 --- Modules/Numerics/Statistics/include/itkHistogram.hxx | 2 -- 1 file changed, 2 deletions(-) diff --git a/Modules/Numerics/Statistics/include/itkHistogram.hxx b/Modules/Numerics/Statistics/include/itkHistogram.hxx index 8d190612ca4..aafcf2107ac 100644 --- a/Modules/Numerics/Statistics/include/itkHistogram.hxx +++ b/Modules/Numerics/Statistics/include/itkHistogram.hxx @@ -176,8 +176,6 @@ Histogram::Initialize(const SizeType & size) this->m_OffsetTable[i + 1] = num; } - this->m_TempIndex.SetSize(this->GetMeasurementVectorSize()); - m_NumberOfInstances = num; // adjust the sizes of min max value containers