Christian Holler (:decoder) Reporter
	Description • 11 years ago

The following parts of our codebase show signed integer overflows at runtime (when running our tests, based on mozilla-inbound 8bf84234319a):

# Format is: Location, Error, Test that triggered it (each in a single line)

gfx/thebes/gfxXlibSurface.cpp:126
runtime error: signed integer overflow: 100000000 * 32 cannot be represented in type 'int'
file:///builds/slave/test/build/tests/reftest/tests/content/canvas/crashtests/780392-1.html


This looks like a trivial thing to me, maybe we can just cast to unsigned here? Getting rid of this error is helpful for testing :)

Putting needinfo on justin according to hg blame for that particular code. If you're not the right person to take a look, could you please suggest someone? :) Thanks!

	Trevor Saunders (:tbsaunde)
	Comment 1 • 11 years ago

lets need info a random gfx person instead (I'm curious how dolske ever got blaime here)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 2 • 11 years ago

The wonders of blame.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 3 • 11 years ago

This doesn't really fix the problem (unless there is a canvas size limit in the first place), but it fixes the test.  It works only if Cairo surface depth is always a byte multiple (e.g., you can't have 4 bit depth).  CheckedInt solution is probably the right one, but I wanted to put this one out there see what the thoughts are.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 4 • 11 years ago

Note that we assume (and assert) that the cairo surface depth is a multiple of bytes.  If that fails, we will not record the memory usage correctly.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 5 • 11 years ago

Comment on attachment 813563 [details] [diff] [review]
Avoid integer overflow using CheckedInt and rearranging the number of bytes computation.

Additional review for Jeff: what are the chances of having Xlib cairo surface depth not a byte multiply (e.g., 1 bit per pixel)?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 6 • 11 years ago

Comment on attachment 813563 [details] [diff] [review]
Avoid integer overflow using CheckedInt and rearranging the number of bytes computation.

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

Enough questions/comments to justify a second round.

::: gfx/thebes/gfxXlibSurface.cpp
@@ +121,5 @@
>  {
>      NS_ASSERTION(!mPixmapTaken, "I already own the Pixmap!");
>      mPixmapTaken = true;
>  
> +    int bitDepth = cairo_xlib_surface_get_depth(CairoSurface());

If you make this unsigned, then the compiler will be able to optimize the division by 8 below as a bitshift.

The existing code doesn't have that optimization though, so it's up to you and can be a separate patch.

@@ +122,5 @@
>      NS_ASSERTION(!mPixmapTaken, "I already own the Pixmap!");
>      mPixmapTaken = true;
>  
> +    int bitDepth = cairo_xlib_surface_get_depth(CairoSurface());
> +    NS_ASSERTION((bitDepth % 8) == 0, "Memory used not recorded correctly");

Just to be clear, NS_ASSERTIONS mostly just (by default) print to stderr on debug builds, so they generally don't get noticed. If you care about this getting noticed, consider using MOZ_ASSERT, which crashes debug builds.

@@ +129,5 @@
>      // pixel.
> +    CheckedInt32 totalBytes = CheckedInt32(mSize.width) * CheckedInt32(mSize.height) * (bitDepth/8);
> +    if (totalBytes.isValid()) {
> +        RecordMemoryUsed(totalBytes.value());
> +    }

When totalBytes is invalid, you know that the image is taking at least UINT32_MAX bytes, so why not report that?

	Karl Tomlinson (:karlt)
	Comment 7 • 11 years ago

Note that the pixmap memory recorded is only an approximation.
For example a depth 24 pixmap will use 4 bytes per pixel in video or server cpu memory and perhaps 4 in each.

Pixmaps won't be larger than 0x7fff x 0x7fff.
There will be no pixmap to TakePixmap() if a cairo xlib surface is created with a larger surface and the XFreePixmap in ~gfxXlibSurface would trigger a fatal error.

(In reply to Milan Sreckovic [:milan] from comment #5)
> What are the chances of having Xlib cairo
> surface depth not a byte multiply (e.g., 1 bit per pixel)?

There are pict formats with depth 1 and 4 and there is cairo_xlib_surface_create_for_bitmap(), so this is theoretically possible, but I doubt any clients are using these.  I suggest just warning if it is not a multiple of 8.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 8 • 11 years ago

(In reply to Benoit Jacob [:bjacob] from comment #6)
> ...
> >  
> > +    int bitDepth = cairo_xlib_surface_get_depth(CairoSurface());
> 
> If you make this unsigned, then the compiler will be able to optimize the
> division by 8 below as a bitshift.

Fair enough - Cairo stores depth as int (doesn't seem to check for <0), the function returns int, but this is probably the last place we have to worry about if negative depth somehow makes it through.  I'll add a comment to that effect.  


> 
> The existing code doesn't have that optimization though, so it's up to you
> and can be a separate patch.
> 
> @@ +122,5 @@
> >      NS_ASSERTION(!mPixmapTaken, "I already own the Pixmap!");
> >      mPixmapTaken = true;
> >  
> > +    int bitDepth = cairo_xlib_surface_get_depth(CairoSurface());
> > +    NS_ASSERTION((bitDepth % 8) == 0, "Memory used not recorded correctly");
> 
> Just to be clear, NS_ASSERTIONS mostly just (by default) print to stderr on
> debug builds, so they generally don't get noticed. If you care about this
> getting noticed, consider using MOZ_ASSERT, which crashes debug builds.

Will do.

> 
> @@ +129,5 @@
> >      // pixel.
> > +    CheckedInt32 totalBytes = CheckedInt32(mSize.width) * CheckedInt32(mSize.height) * (bitDepth/8);
> > +    if (totalBytes.isValid()) {
> > +        RecordMemoryUsed(totalBytes.value());
> > +    }
> 
> When totalBytes is invalid, you know that the image is taking at least
> UINT32_MAX bytes, so why not report that?

Good question.  I don't actually expect this to get hit anymore, not with the modified computation that first gets byte depth, then multiplies things through.  But, more to the point, RecordMemoryUsed() takes int and just adds it to other int, so we could easily be creating an overflow there.  I'd rather not add the "else" part in this case, but put in an assert.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 9 • 11 years ago

The assert type and unsigned int have been changed, but the last comment on the usage of UINT32_MAX for the result I left the way it was, with the better code comments explaining why I think we shouldn't go there.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 10 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=323786ec8b59

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/d23ba68d326e

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

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