1251742 - avoid overflow in computing area of surface sizes in SurfaceCache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This patch is before the patch for bug 1251091 in my patch queue, so I'll land this patch first.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8724244 [details] [diff] [review]
patch

Review of attachment 8724244 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfaceCache.cpp
@@ +331,5 @@
>          static_cast<int64_t>(idealKey.Size().height);
> +      int64_t surfaceArea = static_cast<int64_t>(aSurfaceKey.Size().width) *
> +        static_cast<int64_t>(aSurfaceKey.Size().height);
> +      int64_t bestMatchArea = static_cast<int64_t>(bestMatchKey.Size().width) *
> +        static_cast<int64_t>(bestMatchKey.Size().height);

There's a lot of repetition here (the pattern as a whole is repeated 3 times, and each key.Size() is repeated twice within each instance of the pattern).  Makes for slightly-hard reading, and possibility for easy-to-miss mistakes.

So, I'd prefer we factored out a trivial helper function, defined right above this method if you like, e.g.

static int64_t
AreaOfIntSize(IntSize aSize) {
...
}

Then this could just be:
  int64_t idealArea = AreaOfIntSize(idealKey.Size());
  int64_t surfaceArea = AreaOfIntSize(surfaceKey.Size());
  int64_t bestMatchArea = AreaOfIntSize(bestMatchKey.Size());

r=me with that.

Comment 3

[Daniel Holbert [:dholbert]]

Sorry, I should've had "IntSize& aSize" in the helper-function signature (passing by reference instead of by copy).

Comment 4

[Timothy Nikkel (:tnikkel)]

Good idea. Made that change. Thanks!

Comment 5

[Timothy Nikkel (:tnikkel)]

I also had to make SurfaceKey::Size return a const IntSize& (instead of IntSize), and AreaOfIntSize take a const IntSize&.

Comment 6

[Daniel Holbert [:dholbert]]

Makes sense. Thanks!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ae782bcc00

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4ae782bcc00

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8724244 [details] [diff] [review]
patch

Need this for bug 1251091.

Approval Request Comment
[Feature/regressing bug #]: bug 1119774 added this code
[User impact if declined]: needed for bug 1251091
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: safe
[String/UUID change made/needed]: none

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724244 [details] [diff] [review]
patch

More surface cache work, please uplift to aurora.
Timothy, hopefully you can keep an eye out for new regressions/crashes in 46 that might be related as we go into early beta.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b950d8d294c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b950d8d294c