Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

N-Dekker
Copy link
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
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
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
Member

thewtex commented Aug 10, 2020

Merged into release and master.

@thewtex thewtex closed this Aug 10, 2020
@N-Dekker
Copy link
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
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