1298548 - Prevent overflow in logical size / cost calculations related to the ImageLib surface cache

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

To compute the "cost" of storing a cache entry in the ImageLib surface cache, we use a heuristic that generally involves computing the area of a surface. That in turn involves multiplying a width by a height. When this computation overflows, the cost value can wrap around to something very small, which could lead us to allocate much more memory for surfaces than we're supposed to. We can fix this by performing the calculations via CheckedInt.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Use CheckedInt to prevent overflow in SurfaceCache::CanHold().

CanHold() just redirects to ComputeCost(), so the fix goes there. We perform the
math using CheckedInt32 because IntSize uses int32_t as the underlying type. We
then convert to CheckedInt<Cost> and return the value if it's valid.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Use CheckedInt to prevent overflow in ISurfaceProvider::LogicalSizeInBytes() implementations.

This part makes the same alteration to the various LogicalSizeInBytes()
implementations. We should probably consolidate all this code, but it's not
completely trivial so I decided to leave that for future work rather than delay
getting this bug fixed.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8785490 [details] [diff] [review]
(Part 1) - Use CheckedInt to prevent overflow in SurfaceCache::CanHold().

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

r=me

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8785491 [details] [diff] [review]
(Part 2) - Use CheckedInt to prevent overflow in ISurfaceProvider::LogicalSizeInBytes() implementations.

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

r=me, one nit:

::: image/ISurfaceProvider.cpp
@@ +67,5 @@
>  
>  size_t
>  SimpleSurfaceProvider::LogicalSizeInBytes() const
>  {
> +  const IntSize size = mSurface->GetSize();

You're removing the "gfx::" prefix for brevity here (you're replacing gfx::IntSize with just IntSize), BUT this file has no "using" decl to provide the gfx namespace.  (The other files in this patch do have that, but this one does not.)

Please add...
using namespace mozilla::gfx;
...to the top of this file, to provide that & to avoid introducing a unified-build compilation-error timebomb. :)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.