vnl_sparse_matrix::mult - size of q buffer should actually be (this->rows())*pcols. #4

daniel-perry · 2014-07-24T00:30:37Z

I ran into a subtle bug in vnl_sparse_matrix::mult(), where the code assumes the p and q buffers are the same size (prows_pcols). However the size of q should really be (this->rows())_pcols.

ie, in matrix multiplication an (a,b) size matrix multiplied with a (b,c) size matrix results in a (a,c) size matrix, not (b,c).

blowekamp · 2014-07-30T12:54:13Z

Hello,

Thank you for taking the time to contribute upstream to ITK. Can you please submit a patch to gerrit for review:
http://www.itk.org/Wiki/ITK/Git/Develop

Additionally, a test to illustrate the problem and demonstrate correctness of your solution should be added.

From Coverity Scan: CID 1081058 (#2-1 of 4): Out-of-bounds write (OVERRUN) CID 1081058 (#3 of 4): Out-of-bounds write (OVERRUN) CID 1081058 (#4 of 4): Out-of-bounds write (OVERRUN) 6. overrun-local: Overrunning array of 2 4-byte elements at element index 2 (byte offset 8) by dereferencing pointer "x + j". Change-Id: Ic3669d48624559509f594dc7f82f4caa2cd1b601

Fix coverity resource leak bug reported as: CID 1081080 (#4 of 4): Resource leak (RESOURCE_LEAK) 6. leaked_storage: Variable "quadEdge3" going out of scope leaks the storage it points to Change-Id: I18ab92eac58aa131c73321c351e47721af856053

address 3 coverity issues related to accessing m_Visitors out of bounds. CID 1081041 (#4-1 of 4): Out-of-bounds read (OVERRUN) 3. overrun-local: Overrunning array of 9 8-byte elements at element index 9 (byte offset 72) by dereferencing pointer "&this->m_Visitors[id]" Change-Id: Idf7e306c489a30e429ceff74d61849d5bbdb664f

CID 1103199 (#1 of 4): Destination buffer too small (STRING_OVERFLOW) string_overflow: You might overrun the 35 byte destination string "hdr->hospital" by writing 2049 bytes from "tmpStr" CID 1103199 (#2 of 4): Destination buffer too small (STRING_OVERFLOW) string_overflow: You might overrun the 32 byte destination string "hdr->scanId" by writing 2049 bytes from "tmpStr". CID 1103199 (#3 of 4): Destination buffer too small (STRING_OVERFLOW) string_overflow: You might overrun the 32 byte destination string "hdr->patientId" by writing 2049 bytes from "tmpStr". CID 1103199 (#4 of 4): Destination buffer too small (STRING_OVERFLOW) string_overflow: You might overrun the 32 byte destination string "hdr->date" by writing 2049 bytes from "tmpStr". In all cases, strcpy was replaced with strncpy. Change-Id: I9955d3c34970b89dfddaa233dfbcb8943166f1be

daniel-perry · 2014-07-30T14:46:52Z

Hi,

Thanks for the response.. I was wondering why there were so few pull
requests on github..

I have now registered on ITK's Gerrit and am all setup that way, however
I'm unsure where I should put the demonstration tests?

On Wed, Jul 30, 2014 at 6:54 AM, Bradley Lowekamp notifications@github.com
wrote:

Hello,

Thank you for taking the time to contribute upstream to ITK. Can you
please submit a patch to gerrit for review:
http://www.itk.org/Wiki/ITK/Git/Develop

Additionally, a test to illustrate the problem and demonstrate correctness
of your solution should be added.

—
Reply to this email directly or view it on GitHub
#4 (comment)
.

blowekamp · 2014-07-30T15:04:00Z

Were you using this class directly or through some part of ITK?

It looks like it would be best tested here:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/VNL/src/vxl/core/vnl/tests/test_sparse_matrix.cxx

And important to ensure you patch doesn't have any side effects. This seems like fairly important code for the sparse matrix so I'm a little skeptical of the change. Creating a test which demonstrates the necessity and correctness of the change is important.

Also note while this should be put up to ITK gerrit for review it may need to be pushed upstream to VXL too.

Thanks for contributing!
Brad

On Jul 30, 2014, at 10:46 AM, Daniel Perry notifications@github.com wrote:

Hi,

Thanks for the response.. I was wondering why there were so few pull
requests on github..

I have now registered on ITK's Gerrit and am all setup that way, however
I'm unsure where I should put the demonstration tests?

On Wed, Jul 30, 2014 at 6:54 AM, Bradley Lowekamp notifications@github.com
wrote:

Hello,

Thank you for taking the time to contribute upstream to ITK. Can you
please submit a patch to gerrit for review:
http://www.itk.org/Wiki/ITK/Git/Develop

Additionally, a test to illustrate the problem and demonstrate correctness
of your solution should be added.

—
Reply to this email directly or view it on GitHub
#4 (comment)
.

—
Reply to this email directly or view it on GitHub.

daniel-perry · 2014-07-30T15:26:37Z

Ok thanks.

I was using the class directly, I needed a sparse matrix and couldn't find one in ITK proper...
My use case is pretty rare, since I then needed to multiply with a dense matrix.. however the only supplied option for that is the .mult() method, which I found causes a seg fault if the resulting matrix is smaller than the right matrix in the multiply (it assumes the resulting matrix buffer is the same size as the left matrix in the multiplication).

The fix is pretty straightforward (use number of rows of left matrix not right matrix when zeroing out the resulting matrix buffer q), but I'll add the tests to make sure everything is working as expected.. certainly don't want to break anything.

ps - I also sent the patch to VXL directly (via their github repo), they said it looks ok but are waiting for those responsible for sparse matrix code to look it over before merging.

daniel-perry · 2014-07-30T18:48:15Z

I submitted the fix and test to garret.. adding the test was a good idea, because it revealed another line that needed to be changed.

I wasn't sure who to add as reviewers... so I just put the two people who have edited those files the most.

iMichka · 2014-07-31T21:53:04Z

Seems the patch was merged. Can you close this pull request please ? :)

Thanks

…tion Alexander Schmidt-Richberg (1): BUG: Small bugs fixed to avoid compiler warnings (unsigned int and unused variable). Francois Budin (6): ENH: Replacing test data with MD5 STYLE: Improve style to pass KWStyle test DOC: Missing backslash in documentation COMP: Remove compilation warnings from Visual Studio COMP: Correct warning due to locally defined symbol Merge pull request InsightSoftwareConsortium#9 from fbudin69500/locally_defined_symbol aschmiri (5): Merge pull request #3 from thewtex/doxygen-warnings Merge pull request #4 from fbudin69500/DOC_missing_backslash_in_documentation Merge pull request #5 from fbudin69500/ENH_replace_test_data_with_MD5 Merge pull request #6 from fbudin69500/ImproveStyle Merge pull request InsightSoftwareConsortium#7 from fbudin69500/remove_VS_warning_cherry_picked Change-Id: Ibb959bc84e01edb6c3b78e0984d9f029992ff5af

…eClasses ENH: Remove tree classes

Hello, I was having trouble with an external python calling the itk function as shared below. The ?? sometimes indicates a callback but to be sure I searched all instances of H5VL_group_close and only H5G__close_cb was calling it and it is currently unmangled. I will test this soon and report back. <details> ``` #0 0x00007febfdaa8b49 in itk_H5CX_get_vol_wrap_ctx () from /home/gabriel/g/bin/../lib/GeoSlicer-5.1/libitkhdf5-shared-5.3.so.1 InsightSoftwareConsortium#1 0x00007febfdd23a44 in itk_H5VL_set_vol_wrapper () from /home/gabriel/g/bin/../lib/GeoSlicer-5.1/libitkhdf5-shared-5.3.so.1 InsightSoftwareConsortium#2 0x00007febfdd1870b in itk_H5VL_group_close () from /home/gabriel/g/bin/../lib/GeoSlicer-5.1/libitkhdf5-shared-5.3.so.1 InsightSoftwareConsortium#3 0x00007febfdb4b11d in ?? () from /home/gabriel/g/bin/../lib/GeoSlicer-5.1/libitkhdf5-shared-5.3.so.1 InsightSoftwareConsortium#4 0x00007fead7b7d480 in H5I_dec_ref () from /home/gabriel/g/lib/Python/lib/python3.9/site-packages/h5py/../h5py.libs/libhdf5-346dbfc8.so.200.1.0 InsightSoftwareConsortium#5 0x00007fead7b7d544 in H5I_dec_app_ref () from /home/gabriel/g/lib/Python/lib/python3.9/site-packages/h5py/../h5py.libs/libhdf5-346dbfc8.so.200.1.0 InsightSoftwareConsortium#6 0x00007fead7b79fd1 in H5Idec_ref () from /home/gabriel/g/lib/Python/lib/python3.9/site-packages/h5py/../h5py.libs/libhdf5-346dbfc8.so.200.1.0 ``` </details>

Found with thread sanitizer (TSan) running the `itkANTSNeighborhoodCorrelationImageToImageRegistrationTest` unit test. There was already a `m_ANTSAssociateOnceFlag` flag for use with `std::call_once`, so I just used that in this place too (it's already used elsewhere). Partial backtrace from TSan: ``` WARNING: ThreadSanitizer: data race (pid=79176) Write of size 8 at 0x000108f02240 by thread T2: #0 void itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution_impl<itk::ThreadedIndexedContainerPartitioner>(itk::IdentityHelper<itk::ThreadedIndexedContainerPartitioner>, itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx:118 (ITKMetricsv4TestDriver:arm64+0x1004bfe50) #1 itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution(itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.h:235 (ITKMetricsv4TestDriver:arm64+0x1004bf9c0) #2 itk::DomainThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreaderCallback(void*) itkDomainThreader.hxx:123 (ITKMetricsv4TestDriver:arm64+0x100259b6c) #3 std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()::operator()() const itkThreadPool.h:92 (ITKMetricsv4TestDriver:arm64+0x1007d3228) #4 decltype(std::declval<void* (*&)(void*)>()(std::declval<itk::PoolMultiThreader::ThreadPoolInfoStruct*>())) std::__1::__invoke[abi:v160006]<std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()&>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&) invoke.h:394 (ITKMetricsv4TestDriver:arm64+0x1007d31a4) Previous write of size 8 at 0x000108f02240 by thread T14: #0 void itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution_impl<itk::ThreadedIndexedContainerPartitioner>(itk::IdentityHelper<itk::ThreadedIndexedContainerPartitioner>, itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx:118 (ITKMetricsv4TestDriver:arm64+0x1004bfe50) #1 itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution(itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.h:235 (ITKMetricsv4TestDriver:arm64+0x1004bf9c0) #2 itk::DomainThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreaderCallback(void*) itkDomainThreader.hxx:123 (ITKMetricsv4TestDriver:arm64+0x100259b6c) #3 std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()::operator()() const itkThreadPool.h:92 (ITKMetricsv4TestDriver:arm64+0x1007d3228) #4 decltype(std::declval<void* (*&)(void*)>()(std::declval<itk::PoolMultiThreader::ThreadPoolInfoStruct*>())) std::__1::__invoke[abi:v160006]<std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()&>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&) invoke.h:394 (ITKMetricsv4TestDriver:arm64+0x1007d31a4) ```

size of q buffer should actually be (this->rows())*pcols.

6d4c9bf

daniel-perry closed this Aug 1, 2014

HastingsGreer pushed a commit to HastingsGreer/ITK that referenced this pull request Feb 9, 2019

Merge pull request InsightSoftwareConsortium#4 from aylward/RemoveTre…

0ba079b

…eClasses ENH: Remove tree classes

hjmjohnson mentioned this pull request Apr 11, 2020

BUG: Fix mattes metric with sse instructions #1762

Merged

2 tasks

seddins mentioned this pull request Aug 31, 2023

Similarity3DTransform API #4149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vnl_sparse_matrix::mult - size of q buffer should actually be (this->rows())*pcols. #4

vnl_sparse_matrix::mult - size of q buffer should actually be (this->rows())*pcols. #4

daniel-perry commented Jul 24, 2014

blowekamp commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

blowekamp commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

iMichka commented Jul 31, 2014

vnl_sparse_matrix::mult - size of q buffer should actually be (this->rows())*pcols. #4

vnl_sparse_matrix::mult - size of q buffer should actually be (this->rows())*pcols. #4

Conversation

daniel-perry commented Jul 24, 2014

blowekamp commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

blowekamp commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

daniel-perry commented Jul 30, 2014

iMichka commented Jul 31, 2014