Closed Bug 1296731 Opened 5 years ago Closed 5 years ago

Tweak GetAlignedStride parameters and implementation

Categories

(Core :: Graphics, defect)

Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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".
Attachment #8783026 - Attachment is patch: true
Attachment #8783026 - Flags: feedback?(bas)
Assignee: nobody → milan
Whiteboard: [gfx-noted]
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.
Attachment #8783026 - Flags: feedback?(bas) → feedback+
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.
Attachment #8783639 - Flags: review?(bas) → review+
(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.
Attachment #8783026 - Attachment is obsolete: true
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1710e1e21df0
Add parameter to GetAlignedStride and check for overflow. r=bas
https://hg.mozilla.org/mozilla-central/rev/1710e1e21df0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.