| View Issue Details [ Jump to Notes ] | [ Print ] | ||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||
| 0011443 | ITK | public | 2010-11-10 17:38 | 2010-12-06 14:27 | |||||
| Reporter | kentwilliams | ||||||||
| Assigned To | Matthew McCormick | ||||||||
| Priority | normal | Severity | crash | Reproducibility | always | ||||
| Status | closed | Resolution | fixed | ||||||
| Platform | OS | OS Version | |||||||
| Product Version | ITK-4-A1 | ||||||||
| Target Version | Fixed in Version | ||||||||
| Summary | 0011443: itkSampleToHistogramFilterTest7 crashes if Floating Point Exceptions are turned on. | ||||||||
| Description | There were 2 issues. The first is fixed with the attached patch -- the initial loop setting up the sample contained expressions that overflowed the Measurement type. The second happened because SetHistogramBinMaximum/Minimum were set to the maximum/minimum for the Measurement type -- in itk::Histogram::Initialize it computes the interval between the upper and lower bounds -- essentially std::traits<signed int>::max() - std::traits<signed int>::min() -- and then uses it later in an an expression. So the BinMin and BinMax are set to values that cannot fit in a signed int. See itkHistogram.txx -- line 248 (in ITKV4) where it computes the interval, 254-257 where it uses the interval to set the bin min/max. The numeric exception occurs at line 309 of itkSampleHistogramFilter.txx If I a use a maximum of itk::NumericTraits<MeasurementType>::max() - 127 instead, no overflow occurs. Since I don't know exactly what this test intends, I don't know if that's an acceptable solution. | ||||||||
| Tags | No tags attached. | ||||||||
| Resolution Date | |||||||||
| Sprint | |||||||||
| Sprint Status | backlog | ||||||||
| Attached Files |  itkSampleToHistogramFilterTest7.cxx.patch [^] (1,067 bytes) 2010-11-10 17:38 [Show Content]
| ||||||||
| Relationships | |
| Relationships |
| Notes | |
|
(0023154) kentwilliams (developer) 2010-11-10 17:40 |
Karthik, I assigned this to you since you wrote the original test. If you can suggest how the test can be made numerically reasonable and still exercise the filter properly, I'd be glad to implement the change and do the Gerrit/Stage thing. |
|
(0023383) kentwilliams (developer) 2010-11-18 09:41 |
Gaetan, you have a fix logged in Gerrit related to this problem. Can you make sure it addresses this problem? |
|
(0023565) Hans Johnson (developer) 2010-11-28 11:06 |
Please review source code and determine if this has already been addressed. |
|
(0023698) kentwilliams (developer) 2010-12-03 11:20 |
Fix posted to Gerrit http://review.source.kitware.com/#change,498 [^] This patch stops the numeric exceptions from occuring, but I'm not 100% sure myself about it. I submitted it because no one else has stepped up to look at the problem. The problem occurs because the test was -- frankly -- pretty poorly written. It doesn't properly account for numeric range and subsequently assigned garbage to int variable. The fact that it passed stemmed from the fact that it didn't actually do any sort of reasonable checking of its results. The issue in the Histogram class itself illustrates a problem in using NumericTraits<T>::min() to NumericTraits<T>::max(); namely, that sometimes max()-min() > max(). The SafeAssign method is necessary to get around the fact that at least for some integer types there's a curious problem with casting between int and float types: int x = itk::NumericTraits<int>::max(); // no problem // the following will overflow int x = static_cast<int> (static_cast<double>(itk::NumericTraits<int>::max())); It probably wouldn't hurt for someone who's smarter about numeric analysis to revisit the Histogram class to try and 'bullet-proof' it by design, instead of tiptoing around inevitable overflows. |
|
(0023700) Matthew McCormick (developer) 2010-12-03 11:52 |
Thanks for addressing this Kent, and sorry I did not have time to address it myself. The fix looks good to me. I am not sure what the testing writer was designed the test for either. Ideally, sanity checking code should only be added when it is needed. Otherwise, it would be a bug in the code that uses the class. But if it was in the test, maybe it a common use case, and the robustness is needed. I will test the Gerrit patch. |
|
(0023741) kentwilliams (developer) 2010-12-06 14:27 |
New Gerrit patch 'fixes the fix' http://review.source.kitware.com/#change,520 [^] |
| Notes |
| Issue History | |||
| Date Modified | Username | Field | Change |
| 2010-11-10 17:38 | kentwilliams | New Issue | |
| 2010-11-10 17:38 | kentwilliams | File Added: itkSampleToHistogramFilterTest7.cxx.patch | |
| 2010-11-10 17:38 | kentwilliams | Sprint Status | => backlog |
| 2010-11-10 17:40 | kentwilliams | Note Added: 0023154 | |
| 2010-11-10 17:40 | kentwilliams | Assigned To | => Karthik Krishnan |
| 2010-11-10 17:40 | kentwilliams | Status | new => assigned |
| 2010-11-18 09:41 | kentwilliams | Note Added: 0023383 | |
| 2010-11-18 09:41 | kentwilliams | Assigned To | Karthik Krishnan => Gaetan Lehmann |
| 2010-11-18 09:41 | kentwilliams | Status | assigned => new |
| 2010-11-18 09:42 | kentwilliams | Assigned To | Gaetan Lehmann => Matthew McCormick |
| 2010-11-18 09:42 | kentwilliams | Status | new => assigned |
| 2010-11-28 11:06 | Hans Johnson | Note Added: 0023565 | |
| 2010-12-03 11:20 | kentwilliams | Note Added: 0023698 | |
| 2010-12-03 11:52 | Matthew McCormick | Note Added: 0023700 | |
| 2010-12-06 14:27 | kentwilliams | Note Added: 0023741 | |
| 2010-12-06 14:27 | kentwilliams | Status | assigned => closed |
| 2010-12-06 14:27 | kentwilliams | Resolution | open => fixed |
| Issue History |
| Copyright © 2000 - 2018 MantisBT Team |