Skip to content

BUG: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)"#1951

Closed
N-Dekker wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:ImageMaskSpatialObject-IsInsideInObjectSpace-access-outside-image-buffer
Closed

BUG: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)"#1951
N-Dekker wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:ImageMaskSpatialObject-IsInsideInObjectSpace-access-outside-image-buffer

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Aug 6, 2020

Fixed issue #1950, "ImageMaskSpatialObject tries to access pixels
outside the image buffer (segfault)".

In the original code, TransformPhysicalPointToContinuousIndex did
estimate whether the specified point was inside the image buffer, but
then GetInterpolator()->EvaluateAtContinuousIndex(index) did in some
cases still try to access a pixel outside the image buffer.

This fix avoids using an interpolator. It only accesses a pixel when its
index is inside the buffered region.

The use of an interpolator appears less relevant for an image mask than
for other spatial objects, as for each mask pixel value, it is usually
only interesting to know whether it is zero or non-zero.

Added ImageMaskSpatialObject.CornerPointIsNotInsideMaskOfZeroValues unit
test.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Aug 6, 2020

The test failure that this PR should fix, once it's ready:

D:\a\1\s\Modules\Core\SpatialObjects\test\itkImageMaskSpatialObjectGTest.cxx(324): error: Value of: imageMaskSpatialObject->IsInsideInObjectSpace(cornerPoint)
Actual: true
Expected: false
[ FAILED ] ImageMaskSpatialObject.CornerPointIsNotInsideMaskOfZeroValues (1 ms)

https://dev.azure.com/itkrobotwindow/ITK.Windows/_build/results?buildId=4422&view=logs&j=2d2b3007-3c5c-5840-9bb0-2b1ea49925f3&t=77aad734-2057-5694-9ae2-ee1f5f26eae8&l=252997

… access outside image buffer

Fixed issue InsightSoftwareConsortium#1950, "ImageMaskSpatialObject tries to access pixels
outside the image buffer (segfault)".

In the original code, `TransformPhysicalPointToContinuousIndex` did
estimate whether the specified point was inside the image buffer, but
then `GetInterpolator()->EvaluateAtContinuousIndex(index)` did in some
cases still try to access a pixel outside the image buffer.

This fix avoids using an interpolator. It only accesses a pixel when its
index is inside the buffered region.

The use of an interpolator appears less relevant for an image mask than
for other spatial objects, as for each mask pixel value, it is usually
only interesting to know whether it is zero or non-zero.

Added ImageMaskSpatialObject.CornerPointIsNotInsideMaskOfZeroValues unit
test.
@N-Dekker N-Dekker force-pushed the ImageMaskSpatialObject-IsInsideInObjectSpace-access-outside-image-buffer branch from e7f7448 to cf54e98 Compare August 6, 2020 18:58
@N-Dekker N-Dekker changed the title WIP: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)" BUG: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)" Aug 6, 2020
@N-Dekker N-Dekker marked this pull request as ready for review August 6, 2020 20:49
Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks, @N-Dekker .

I will merge this manually for inclusion in 5.1.1.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Aug 10, 2020

Merged into release and master.

@thewtex thewtex closed this Aug 10, 2020
@N-Dekker
Copy link
Copy Markdown
Contributor Author

@aylward Hi Stephen, because of the holidays (heatwave) I did not have time to ask you for feedback on this issue. I'm very happy to see that Matt merged my PR so quickly already, to avoid those segfaults. But as you are very much involved with spatial objects, is it also OK to you that ImageMaskSpatialObject::IsInsideInObjectSpace no longer makes use of its interpolator?

This PR is certainly OK to me, I just want to be sure it's also fine to you!

@aylward
Copy link
Copy Markdown
Member

aylward commented Aug 20, 2020

@N-Dekker - Hi Neal, This looks good to me! Thanks for checking, and thanks for the fix! I do see arguments either way regarding the use of an interpolator - so not using an interpolator seems reasonable and it would clearly be faster. I appreciate the change!

N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Sep 3, 2020
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Sep 8, 2020
Including a fix of a bug, reported by Theo van Walsum, "Issues with mask in el 5.0.0 when mask extent is smaller then image extent",
https://groups.google.com/forum/#!category-topic/elastix-imageregistration/elastix/lSLWJq9Zcgk

BUG: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)"
InsightSoftwareConsortium/ITK#1951
InsightSoftwareConsortium/ITK@ceb157d
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Sep 8, 2020
Including a fix of a bug, reported by Theo van Walsum, "Issues with mask in el 5.0.0 when mask extent is smaller then image extent",
https://groups.google.com/forum/#!category-topic/elastix-imageregistration/elastix/lSLWJq9Zcgk

BUG: Fix issue #1950, "ImageMaskSpatialObject tries to access pixels outside the image buffer (segfault)"
InsightSoftwareConsortium/ITK#1951
InsightSoftwareConsortium/ITK@ceb157d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants