GetAlignedStride template function adjusts the integer argument to be a multiple of the templated value (currently power of two). In all but two instances, the callers take the width, multiply it by bytes per pixel, then call this function. In a few places, the callers check if that multiplication would integer overflow, nobody checks if the adjustment to the multiple of templated value would. In some cases we "know" the arguments are checked before, but most of the time we don't. The proposal is to change the signature of the function to take the width and the bytes per pixel, and check for overflow.

Comment on attachment 8783026 [details] [diff] [review] Change the signature and check for overflow We could leave both versions of the function around, but I wanted to make sure people don't keep using it the "old way".

Comment on attachment 8783026 [details] [diff] [review] Change the signature and check for overflow Review of attachment 8783026 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable to me.

Comment on attachment 8783639 [details] Bug 1296731: Add parameter to GetAlignedStride and check for overflow. https://reviewboard.mozilla.org/r/73368/#review71678 ::: gfx/2d/Tools.h:236 (Diff revision 1) > -int32_t GetAlignedStride(int32_t aStride) > +int32_t GetAlignedStride(int32_t aWidth, int32_t aBytesPerPixel) > { > static_assert(alignment > 0 && (alignment & (alignment-1)) == 0, > "This implementation currently require power-of-two alignment"); > const int32_t mask = alignment - 1; > - return (aStride + mask) & ~mask; > + CheckedInt<int32_t> stride = CheckedInt<int32_t>(aWidth) * aBytesPerPixel + mask; Provided all the promotion rules make sense this is all good. But it may be safer/easier to make all the numbers CheckedInts just for good measure.

(In reply to Bas Schouten (:bas.schouten) from comment #4) > > + CheckedInt<int32_t> stride = CheckedInt<int32_t>(aWidth) * aBytesPerPixel + mask; > > Provided all the promotion rules make sense this is all good. But it may be > safer/easier to make all the numbers CheckedInts just for good measure. Will do. There is no CheckedInt -> int automatic conversion, so the above would fail if CheckedInt's didn't get constructed, from what I can tell, but what you suggest happens anyway, so may as well make it explicit.

Pushed by msreckovic@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1710e1e21df0 Add parameter to GetAlignedStride and check for overflow. r=bas