BUG: Guard division-by-zero NaN and UB-cast sites from issue 6575#6589
Open
hjmjohnson wants to merge 10 commits into
Open
BUG: Guard division-by-zero NaN and UB-cast sites from issue 6575#6589hjmjohnson wants to merge 10 commits into
hjmjohnson wants to merge 10 commits into
Conversation
A constant included region made histogramSlope zero, so the histogram bin index became 0.0/0.0 = NaN whose float-to-int conversion is undefined behavior feeding unchecked indexing. Independently, the else-if min/max scan never assigned binMinimum when the traversed values were strictly increasing. Make the scans independent and degrade sharpening to an identity mapping when the intensity range is degenerate. Issue InsightSoftwareConsortium#6575, item B2.
A cluster ending an iteration with zero members divided its accumulator by zero, leaving NaN spatial components whose later float-to-integer rounding is undefined behavior feeding image reads. Issue InsightSoftwareConsortium#6575, item B3.
Pn / (Pf - Pn) divided by zero when the input power at a frequency equaled the noise power spectral density constant; the magnitude guard passes on the resulting infinity and NaN reached the output. Return zero for such frequencies: zero estimated signal power means nothing to recover. Issue InsightSoftwareConsortium#6575, item B4.
…tity A constant image gave a zero intensity range, so the normalization divided by zero and every output pixel became NaN. Issue InsightSoftwareConsortium#6575, item B9.
All-background inputs zero the weight image, making the sensitivity denominator 0.0/0.0 = NaN with no guard. Initialize the estimates and keep the previous value when a denominator vanishes. Issue InsightSoftwareConsortium#6575, item B11.
The degenerate-value guard tested the source voxel count while the division is by FP + TN, so a label covering the whole image passed the guard and produced 0.0/0.0 = NaN. Issue InsightSoftwareConsortium#6575, item B12.
A radius-0 window has one sample, so the unbiased variance divided by zero and every output pixel became sqrt(0.0/0.0) = NaN. Issue InsightSoftwareConsortium#6575, item B20.
A constant input has zero tensors, so the unit-maximum-trace rescale divided by zero and multiplied the tensors into NaN, later driving an undefined float-to-int conversion in the diffusion step count. Issue InsightSoftwareConsortium#6575, items B23 and part of the B23 chain.
The GAUSSIAN fidelity branch and the pure-smoothing path can drive the update negative; casting that to an unsigned pixel type is undefined behavior. Issue InsightSoftwareConsortium#6575, item B27.
A normal variate is unbounded below and the float-to-unsigned conversion of a negative value is undefined behavior; the rejection loop relied on the wrapped result. Issue InsightSoftwareConsortium#6575, item B29.
github-actions
Bot
added
type:Bug
Contributor
|
| Filename | Overview |
|---|---|
| Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.hxx | Adds an identity path for degenerate histogram ranges in N4 sharpening. |
| Modules/Filtering/Denoising/include/itkPatchBasedDenoisingImageFilter.hxx | Clamps update components before converting them back to the output pixel type. |
| Modules/Filtering/ImageCompare/include/itkSTAPLEImageFilter.hxx | Initializes p/q estimates and keeps the prior estimate when a denominator is zero. |
| Modules/Filtering/ImageStatistics/include/itkLabelOverlapMeasuresImageFilter.hxx | Checks the actual false-positive denominator before computing the metric. |
| Modules/Filtering/Smoothing/include/itkBoxUtilities.h | Returns zero sigma for single-sample body and edge windows. |
| Modules/Numerics/Statistics/include/itkGaussianRandomSpatialNeighborSubsampler.hxx | Rejects out-of-bound Gaussian variates before converting them to integer indices. |
| Modules/Segmentation/SuperPixel/include/itkSLICImageFilter.hxx | Keeps the previous centroid when a SLIC cluster has no assigned pixels. |
Reviews (1): Last reviewed commit: "BUG: Reject out-of-bound Gaussian variat..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guards the easy division-by-zero → NaN → undefined-behavior sites catalogued in #6575: ten fixes (items B2, B3, B4, B9, B11, B12, B20, B23, B27, B29), one commit per item, with seven new GoogleTests. Five of the tests demonstrably fail on the pre-fix code; the full test suites of all nine touched modules pass (235/235 locally).
Per-item summary
N4BiasFieldCorrectionImageFilter::SharpenImageSLICImageFilterWienerDeconvolutionImageFilterfunctorPf == Pn→ zero output (was complex division by zero; the magnitude guard passes on inf)AdaptiveEqualizationHistogramSTAPLEImageFilterLabelOverlapMeasuresImageFilterBoxSigmaImageFilter(itkBoxUtilities.h, both body and edge paths)StructureTensorImageFilter(AnisotropicDiffusionLBR)PatchBasedDenoisingImageFilterGaussianRandomSpatialNeighborSubsamplerTest coverage and verification notes
New GoogleTests: N4 constant-image (BiasCorrection — new GTest driver entry), Wiener functor zero-SNR (Deconvolution), AdaptiveHistogramEqualization constant-image + LabelOverlapMeasures whole-image label (ImageStatistics), BoxSigma radius-0 (Smoothing), STAPLE all-background (ImageCompare — new GTest driver block), StructureTensor constant-image (AnisotropicDiffusionLBR).
Fail-before/pass-after was verified locally by rebuilding each test against the pre-fix sources: the AHE, LabelOverlap, BoxSigma, STAPLE, and StructureTensor tests fail before the fix. The N4 and Wiener tests pass pre-fix on arm64/macOS because the underlying UB (NaN→int conversion, libc complex division) happens to be benign there — they pin the now-defined behavior and would catch regressions under UBSan or on other platforms.
Untested-by-design: B3 (an empty SLIC cluster is not deterministically constructible from the public API), B27 and B29 (UB-only guards with no observable defined-behavior change on this platform).
Findings and analysis by the #6575 reporter; see that ledger for the full derivations.