Fix CPU segfault in roi_align when ROI contains NaN coordinates#9541
Open
nileshpatil6 wants to merge 1 commit into
Open
Fix CPU segfault in roi_align when ROI contains NaN coordinates#9541nileshpatil6 wants to merge 1 commit into
nileshpatil6 wants to merge 1 commit into
Conversation
roi_align on CPU segfaults when a ROI contains NaN coordinates. The bounds check in pre_calc_for_bilinear_interpolate and bilinear_interpolate_gradient compares y/x against -1 and height/width, but every comparison against NaN evaluates to false, so a NaN box silently falls through the check instead of being treated as empty. The NaN value then gets truncated to an int, which is undefined behavior in C++ and in practice produces a garbage index that is used to read the input feature map out of bounds. This adds explicit isnan checks alongside the existing range checks so NaN coordinates are treated the same as any other out-of-bounds ROI (zero weights, no read). Added a regression test that reproduces the crash with a NaN ROI on CPU.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9541
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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
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.
Fixes #9273
roi_align on CPU can segfault when a ROI has NaN coordinates. I tracked it down to the bounds check in
pre_calc_for_bilinear_interpolate(torchvision/csrc/ops/cpu/roi_align_common.h) and the equivalent check inbilinear_interpolate_gradient(torchvision/csrc/ops/cpu/roi_align_kernel.cpp, used by the backward pass).Both do something like:
The problem is that any comparison against NaN is false, so a NaN coordinate slides right through this check instead of being caught as out of bounds. Right after that, the code does
(int)yto compute the pixel index, and casting NaN to int is undefined behavior in C++. In practice it produces some garbage integer, which then gets used aspos1/pos2/pos3/pos4to read straight out of the input feature map buffer, which is exactly what the ASAN report in the issue shows.I reproduced the crash locally with the repro script from the issue against the currently released torchvision (0.21.0) and it segfaults reliably.
Fix is small: add explicit
isnanchecks next to the existing range checks in both places, so a NaN ROI gets treated the same way as any other malformed/out-of-bounds ROI (zero weights, index -1, no memory read), instead of silently producing garbage.Added a regression test (
test_nan_roi_coordinates_do_not_crashin test/test_ops.py) that runs roi_align with a NaN ROI on CPU for bothalignedvalues and both sampling_ratio modes.One thing I want to be upfront about: I don't have a C++ toolchain available in the environment I used to write this patch, so I could not actually compile torchvision from source to run the new test against my fix. What I did verify:
If a maintainer or CI can confirm the actual build + test pass, that'd be great, happy to iterate if anything doesn't compile as expected (e.g. if isnan needs a different form for the Half/BFloat16 specializations dispatched via AT_DISPATCH_FLOATING_TYPES_AND_HALF).