For discussion, possible approach to GafferImage::Warp integer overflow - #7070
Draft
danieldresser-ie wants to merge 1 commit into
Draft
For discussion, possible approach to GafferImage::Warp integer overflow#7070danieldresser-ie wants to merge 1 commit into
danieldresser-ie wants to merge 1 commit into
Conversation
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.
I spent some time today debugging a crash at IE which turned out to be due to invalid inputs to a vector warp.
If the pixel offsets are greater than
2e9, then the size of the input bound needed could wrap around, resulting in trying to do lookups within a zero-sized tile cache.We should definitely do better than crashing here, but it's probably debateable what the right approach is. Because of how we handle accesses outside the image using the bounding mode, we theoretically could make everything work consistently for floating point values that aren't representable as integers ... but this would result in some fiddly off-by-one issues: we usually store the bound max as the highest index + 1, which if we allow the index to reach MAX_INT, will wrap. So would we need to limit the indices to be no higher than MAX_INT - 1? It's all doable, but a bit fiddly, and we'd want to make sure we don't slow down the common case in order to handle this exceptional case.
For the moment, I've just done the simple thing, of throwing an exception if Warp is run on out-of-bound inputs, to make sure we at least don't crash, and make this much quicker to debug in the future.