592118 - -moz-element background causes stack pointer free

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (may crash Firefox when loaded)

Loading the testcase triggers:

* A stack pointer free, which makes malloc complain.

* Assertion failed: (CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&cr->ref_count)), function _moz_cairo_destroy, file gfx/cairo/cairo/src/cairo.c, line 327.

Comment 1

[Jesse Ruderman]

Attachment: valgrind's explanation

Comment 2

[Jesse Ruderman]

Attachment: gdb's explanation

Comment 3

[Jesse Ruderman]

With an opt build, I get a crash with an incomplete stack.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

There's a stack-allocated gfxContext here:

http://hg.mozilla.org/mozilla-central/file/tip/gfx/thebes/gfxUtils.cpp#l251

this is a no-no and is probably causing the problem.  We should really do something to ensure that these things never get allocated on the stack; I'm not quite sure how to do that, though, other than introducing helper static allocation functions which is quite ugly.

(Explanation: gfxContext & gfx*Surface share the object's refcount with the underlying cairo object's refcount.  Upon initial creation, the cairo object has an initial ref, while the gfx* object does not, per xpcom semantics.  So the gfx* object waits until the first AddRef and ignores it, thus syncing up the two.  I guess a way that we can assert on this is to make sure that an AddRef happened at some point in the destructor... then again, I thought we handled this case correctly, too.)

Comment 5

[Jesse Ruderman]

Maybe a static analysis that's the opposite of NS_STACK_CLASS? ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do we need to block b5 on this?  Or just b6?

Comment 7

[Markus Stange [:mstange]]

We have loads of stack-allocated gfxContexts:
http://mxr.mozilla.org/mozilla-central/search?string=gfxContext+[^\*]&regexp=on&tree=mozilla-central

Do we need to fix all of them?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, more details -- in the case of gfxContext, this is only bad if the gfxContext is passed to something else.  Because then you have this situation:

gfxContext cx;
Foo(&cx);
cx.Stuff();

Foo(gfxContext *context) {
  nsRefPtr<gfxContext> cx = context;

  ...
}

gfxContext will get destroyed at the end of Foo, since all outstanding references to it will be dead, even though it will still be on the stack; bad stuff will result.

If the context isn't passed to anything, then it should be safe.  But I still think we should fix all of those; they seem to be just in SVG and imglib, so we should make sure folks working in those areas are aware of this.  If needed, we could also maybe introduce a gfxStackContext or something, with gfxContext inheriting from it and just adding refcounting.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although it's a little unfair, I'm going to ask Markus to fix it ...

Comment 11

[Markus Stange [:mstange]]

What's the policy on checking in crashtests for security sensitive bugs in betas? Wait until the fixed beta is out?

Comment 12

[Markus Stange [:mstange]]

Attachment: v1

I audited all stack-allocated gfxContexts and only changed those that get passed to other functions.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 474014 [details] [diff] [review]
v1

Thanks a ton! Don't forget to add a crashtest!

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, I missed comment #11. I think you can just check in the test anytime.

Comment 15

[Johnny Stenback (:jst)]

Is this ready to land? If not, what's left?

Comment 16

[Markus Stange [:mstange]]

Attachment: with crashtest

Now it's ready to land.

Comment 17

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/d27d71099498