826980 - Crash with large canvas, stroke

Status: RESOLVED WORKSFORME

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes debug; may hang nightly)

In a debug build:
  * Under mozilla::dom::CanvasRenderingContext2D::Stroke:
    * mozilla::gfx::DrawTargetCG::Init encounters OOM, but does not crash
    * CGBlt_copyBytes (in CoreGraphics) crashes accessing a bogus address

In a nightly build:
  * Might hang.

Comment 1

[Jesse Ruderman]

Attachment: stacks (gdb)

Comment 2

[Jesse Ruderman]

Attachment: stack (minidump_stackwalk on a Tinderbox debug build)

Comment 3

[Jesse Ruderman]

Tested on Mac OS X 10.7.5.

Comment 4

[Daniel Veditz [:dveditz]]

If we're really carrying on and then using something we failed to allocate that would be sec-critical bad, but can't tell if that's really the case.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

The code is certainly suspect; on the Mac, for instance, we follow the failed "surface size check" with
unsigned int width = static_cast<unsigned int>(-1);
and on the windows it may assume you have a 1 pixel image running top down.  So, I'm probably not reading the code correctly, as it surely can't be this bad.

Comment 6

[Daniel Veditz [:dveditz]]

I don't see mCG initialized to nullptr anywhere in the constructor and it looks like there might be paths where it ends up being used uninitialized. There are asserts that mCG isn't null at a couple of places, but that seems to assume mCG starts out as null (in a couple of places it's checking null as an error return value, but not always).

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this a dup of bug 818241?

Comment 8

[Jesse Ruderman]

I can still reproduce this bug, so no.

Comment 9

[Jesse Ruderman]

But only on Mac OS X 10.7, not 10.8?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

BenWa, have you had a chance to look at this? We're trying to sort critical and high within six weeks of being identified as gfx, and that's tomorrow :-)

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 12

[Jeff Muizelaar [:jrmuizel]]

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

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

::: gfx/2d/DrawTargetCG.cpp
@@ +912,5 @@
>    if (aData == nullptr && aType != BACKEND_COREGRAPHICS_ACCELERATED) {
>      // XXX: Currently, Init implicitly clears, that can often be a waste of time
>      mData = calloc(aSize.height * aStride, 1);
> +    if (!mData) {
> +      mColorSpace = nullptr;

This leaks mColorSpace

@@ +936,5 @@
>      // and we will fallback to software below
>      mData = nullptr;
>    }
>  
>    if (!mCg || aType == BACKEND_COREGRAPHICS) {

The control flow here has become pretty confusing. Please fix it up.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Did the patch review cycle break down here or something else?

Comment 14

[Benoit Girard (:BenWa)]

Opps. I'll finish this up!

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch

better control flow.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

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

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

::: gfx/2d/DrawTargetCG.cpp
@@ -898,1 @@
>      mColorSpace = nullptr;

It would be better to split out Init into different functions for the Backend types instead of trying to munge both of them together. Unfortunately we'll need to duplicate the size check. However, I think that's ok.

@@ +921,5 @@
> +      mColorSpace = nullptr;
> +      mCg = nullptr;
> +      mData = nullptr;
> +      return false;
> +    }

This should move up close to the return at the top of the function.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 741885 [details] [diff] [review]
patch v2

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

::: gfx/2d/DrawTargetCG.cpp
@@ +189,1 @@
>    }

You could drop the return nullptr to the bottom of the function avoiding the need to duplicate it.

Comment 19

[Benoit Girard (:BenWa)]

Attachment: patch v2 (Rebased)

Comment 20

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=8cd3dd38324b

Comment 21

[Jesse Ruderman]

That try job failed.  BenWa, are you going to try again?

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Did you mean to CC the other Benoit ? ;-)

Comment 23

[Al Billings [:abillings - ex-MoCo]]

One cannot have too many Benoits but yes.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Benoit *bump* - ready for another try run?

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Hey, can we get another try run and maybe get this old sec-high security issue fixed?

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this still a problem?  That code has changed since then, and I can't reproduce the crash or the hang.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Jesse, can you still reproduce this?

Comment 28

[Jesse Ruderman]

Attachment: 10.9 stack (lldb)

Yes, the testcase still crashes debug builds for me.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Jesse Ruderman from comment #28)
> Created attachment 8338716 [details]
> 10.9 stack (lldb)
> 
> Yes, the testcase still crashes debug builds for me.

Milan, Jesse, please report to the octagon.

Comment 30

[Jesse Ruderman]

I can still reproduce now that I'm on Mac OS X 10.9, fwiw.

Comment 31

[Andrew McCreight [:mccr8]]

Benoit, are you going to be able to finish this soon, or should we find somebody else?  The bug is over a year old.

Comment 32

[Benoit Girard (:BenWa)]

This bug is important but I haven't found the time to deal with it. We're pushing an important feature on b2g and I can't find time to look at this.

If we can reassign that would be great.

Comment 33

[Andrew McCreight [:mccr8]]

Is there somebody else who could work on this, Milan?  Thanks.

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

Dan, can you reproduce this?  I instead just get:
Surface size too large (allocation size would overflow int32_t)!
message and nothing bad happens.
Clean profile?

Comment 35

[u480271]

I'll take a look at trying to repro.

Comment 36

[u480271]

Tested nightly on OSX 10.6.8 & 10.9.2 as well as a debug build. All builds were fine. Does this bug need to be marked WFM?

Comment 37

[Andrew McCreight [:mccr8]]

Can you reproduce still, Jesse?

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

Still WFM, but I will try asan build, see what happens.

Comment 39

[Jesse Ruderman]

I can't reproduce this any more. (Still on 10.9.)