Closed Bug 592118 Opened 9 years ago Closed 9 years ago

-moz-element background causes stack pointer free

Categories

(Core :: Graphics, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: mstange)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

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.
Attached file valgrind's explanation
Attached file gdb's explanation
With an opt build, I get a crash with an incomplete stack.
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.)
Maybe a static analysis that's the opposite of NS_STACK_CLASS? ;)
Do we need to block b5 on this?  Or just b6?
blocking2.0: --- → ?
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?
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.
Although it's a little unfair, I'm going to ask Markus to fix it ...
Assignee: nobody → mstange
blocking2.0: ? → final+
Duplicate of this bug: 592086
What's the policy on checking in crashtests for security sensitive bugs in betas? Wait until the fixed beta is out?
Attached patch v1 (obsolete) — Splinter Review
I audited all stack-allocated gfxContexts and only changed those that get passed to other functions.
Attachment #474014 - Flags: review?(roc)
Comment on attachment 474014 [details] [diff] [review]
v1

Thanks a ton! Don't forget to add a crashtest!
Attachment #474014 - Flags: review?(roc) → review+
Oops, I missed comment #11. I think you can just check in the test anytime.
Is this ready to land? If not, what's left?
Attached patch with crashtestSplinter Review
Now it's ready to land.
Attachment #474014 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?] [needs landing]
Whiteboard: [sg:critical?] [needs landing] → [sg:critical?] [needs landing post-b7]
http://hg.mozilla.org/mozilla-central/rev/d27d71099498
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical?] [needs landing post-b7] → [sg:critical?]
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Group: core-security
You need to log in before you can comment on or make changes to this bug.