Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash with gradient, hugeness, "-moz-appearance: statusbar"

VERIFIED FIXED in Firefox 15

Status

()

Core
Widget: Cocoa
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: smichaud)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla17
x86_64
Mac OS X
crash, regression, sec-critical, sec-vector, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ verified, firefox16+ verified, firefox17 verified, firefox-esr1015+ verified)

Details

(Whiteboard: [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?, crash signature)

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 632425 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

5 years ago
Created attachment 632427 [details]
stack trace (gdb)
(Reporter)

Comment 2

5 years ago
Created attachment 632428 [details]
stack trace (breakpad + minidump_stackwalk)

bp-7d2d7035-f989-48ae-bbc8-5efd72120612
(Reporter)

Comment 3

5 years ago
Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?
(Reporter)

Updated

5 years ago
Keywords: sec-critical
(Reporter)

Comment 4

5 years ago
Only crashes if the testcase is the first thing loaded in the session?
Component: Graphics → Widget: Cocoa
QA Contact: thebes → cocoa
(Assignee)

Comment 5

5 years ago
Anyone interested in looking for a regression range?
Keywords: regressionwindow-wanted
(Assignee)

Comment 6

5 years ago
I wonder if this only happens with certain versions of OS X.
(Assignee)

Comment 7

5 years ago
This bug is *very* hard to reproduce.

Loading the testcase first thing in the session doesn't help.

Running ".../firefox [testcaseurl]" or ".../firefox -foreground [testcaseurl]" from the commandline doesn't help.

Just about the only way I can repro a crash is if one has (accidentally) already happened, and Firefox on restart opens to the same page.
(Assignee)

Comment 8

5 years ago
Even making this bug's testcase my home page doesn't help.

I've been testing with the same mozilla-central nightly Jesse used -- yesterday's.
(Assignee)

Comment 9

5 years ago
Without better STR (or a better testcase), I'd say this bug isn't reproducible.
Got it, I think:

Maximize your window before loading the testcase.  I think the window has to be a certain size (or larger) to reproduce the crashes.
With my STR from comment #10, I've found that these crashes only happen on OS X 10.7.

I've also found a regression range -- somewhere between FF 7.0.1 and 8.0.  Later I'll narrow this down to two mozilla-central nightlies.
(Assignee)

Updated

5 years ago
Whiteboard: STR in comment #10
Here's the mozilla-central regression range:

firefox-2011-08-09-03-07-51-mozilla-central
firefox-2011-08-10-03-07-38-mozilla-central

Which translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=08327218cb8b&tochange=04dfb49d3a3d
Nope, it translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87e3ea12ed5d&tochange=04dfb49d3a3d
Markus may have ideas.
93c610640952	Markus Stange — bug 668195 - Use CoreUI to draw window chrome and toolbars. r=josh

That switched us to using the CUI APIs for doing the drawing of this stuff, so whatever APIs we were using before seemed to not have this problem.

The comment in the (current) code says:

  // kCUIWidgetWindowFrame draws a complete window frame with both title bar
  // and bottom bar. We only want the bottom bar, so we extend the draw rect
  // upwards to make space for the title bar, and then we clip it away.
  CGRect drawRect = inBoxRect;
  const int extendUpwards = 40;
  drawRect.origin.y -= extendUpwards;
  drawRect.size.height += extendUpwards;

So in theory with a fullscreen window we could get origin.y being negative.  This is fine from a graphics perspective, and all the CG APIs in the stack should handle it just fine, but who knows if some optimization is being triggered that causes a problem here.

I'd suggest trying to reproduce the bug with a window that's not quite mazimized -- maybe 50 pixels shorter than maximum, but flush up against the bottom of the display leaving space open at the top.
> 93c610640952 Markus Stange — bug 668195 - Use CoreUI to draw window chrome and toolbars. r=josh

I've confirmed this by building with (http://hg.mozilla.org/mozilla-central/rev/93c610640952) at tip (we crash) and with the previously landed patch (http://hg.mozilla.org/mozilla-central/rev/820ac76cec76) at tip (we don't crash).
(In reply to Jesse Ruderman from comment #3)
> Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?

Pretty much. We don't have symbols for it, so we don't know how to get out of it properly.
> drawRect.origin.y -= extendUpwards;
> drawRect.size.height += extendUpwards;

Commenting out these two lines (in current code) doesn't stop the crashes happening.
(Assignee)

Updated

5 years ago
Blocks: 668195
TenFourFox is not vulnerable to this crash. We don't use CoreUI at all because we support 10.4.
Assignee: nobody → smichaud
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox15: --- → +
tracking-firefox16: --- → +
Keywords: regressionwindow-wanted → regression
Whiteboard: STR in comment #10 → STR in comment #10, maybe OS X 10.7 only?
Markus, what sort of documentation (or sample code) for the CoreUI did you work from writing your patch for bug 668195?  Or did you have any?

When I finally get back to this, the first thing I'll need to do is double-check that you're using it correctly (by reverse-engineering how (say) Safari uses it).
(Following up comment #20)

Never mind.  I re-read bug 668195 comment #0 more carefully and now see what you were working from :-)

But from what you say in comment #0 you may not have done much reverse-engineering on 10.7.  So it's possible the CoreUI stuff behaves slightly differently there.

I'll do more checking once I get back to this.
(In reply to Steven Michaud from comment #21)
> So it's possible the CoreUI stuff behaves
> slightly differently there.

Sure, that's definitely possible. But maybe CoreUI window rendering just doesn't anticipate extreme sizes since windows usually aren't that large. I think we should just refuse to draw anything if the draw rect exceeds a certain size.
That was certainly our fix for various image problems -- just cap the size of what we'll deal with.
Keywords: sec-vector
Yes.  The problem is figuring out what size to cap at :-(
Should we be filing a radr bug? (I'm probably confused)
We don't (yet) have enough information for a bug report to Apple.
status-firefox14: affected → wontfix
Steven can we get a mitigation into 15 and 16? Can we cap to the biggest reasonable resolution (times 2 maybe?)
I'll try to hack something out next week.

We can't know exactly where the cap(s) should be without extensive testing (which I won't have time for).  But reasonable guesses should allow me to write a bandaid fix.
I can no longer reproduce these crashes in current trunk code.  So I'm going to write a patch that bails if we'd be asking CUIDraw to draw a rectangle whose "area" (width * height) is larger than an arbitrary value.

We've already been doing this kind of thing for several years in nsNativeThemeCocoa.mm's DrawCellWithScaling() and RenderTransformedHIThemeControl(), without any reports of problems.  So I'll just borrow the value we used there (BITMAP_MAX_AREA == 500000).
Created attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

This OK, Markus?
Attachment #644057 - Flags: review?(mstange)
For what it's worth, here's a tryserver build of my patch from comment #30:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-0ac139f6e412/try-macosx64/firefox-17.0a1.en-US.mac.dmg

Please test with it if you're still able to reproduce this bug in current trunk code.
(In reply to Steven Michaud from comment #30)
> Created attachment 644057 [details] [diff] [review]
> Cap "area" drawn by CUIDraw()
> 
> This OK, Markus?

We're looking to land this in FF15 if deemed low risk. Markus - would you mind reviewing?
Alternately - Benoit, do you want to steal the review from Markus?
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Looks good, but I don't know if it will prevent these crashes completely. For example, I could imagine that we would still crash when drawing a small area (smaller than the cap that this patch imposes) if the transform applied to the element is scaled up by a huge factor. Note that this patch only checks the pre-transform paint size.
Attachment #644057 - Flags: review?(mstange) → review+
> I could imagine that we would still crash when drawing a small area
> (smaller than the cap that this patch imposes) if the transform
> applied to the element is scaled up by a huge factor

Which transform?  The one belonging to the CGContextRef passed into
CUIDraw() (accessible using CGContextGetCTM())?

If so, it'd be easy enough for me to use it to scale the area before
checking its size.
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/970f2d233615

Since this is a bit urgent, let's take my current patch now.  We can alter it later once Markus responds.
https://hg.mozilla.org/mozilla-central/rev/970f2d233615
Flags: in-testsuite-
Target Milestone: --- → mozilla17
(Leaving open per comment #36)

Also, should this have a test?
Flags: in-testsuite- → in-testsuite?
(In reply to comment #38)

The testcase for this bug no longer works in current code.  Until we get one that does, I don't think it'll be possible to write a test.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: --- → fixed
Resolution: --- → FIXED
Please nominate for branch uplift, including ESR10.
tracking-firefox-esr10: --- → 15+
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probable Apple bug triggered by patch for bug 668195 (which uses CoreUI APIs directly)
User impact if declined: Possible exposure to a security exploit
Testing completed (on m-c, etc.): Minimal testing on m-c
Risk to taking this patch (and alternatives if risky): Very low risk (analogous to patch for bug 464589, which has been in tree for five years without causing trouble)
String or UUID changes made by this patch: none
Attachment #644057 - Flags: approval-mozilla-esr10?
Attachment #644057 - Flags: approval-mozilla-beta?
Attachment #644057 - Flags: approval-mozilla-aurora?
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

low risk, approving across all branches.
Attachment #644057 - Flags: approval-mozilla-esr10?
Attachment #644057 - Flags: approval-mozilla-esr10+
Attachment #644057 - Flags: approval-mozilla-beta?
Attachment #644057 - Flags: approval-mozilla-beta+
Attachment #644057 - Flags: approval-mozilla-aurora?
Attachment #644057 - Flags: approval-mozilla-aurora+
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Landed on branches:
http://hg.mozilla.org/releases/mozilla-aurora/rev/cf0d8079a6d8
http://hg.mozilla.org/releases/mozilla-beta/rev/8971a306e188
http://hg.mozilla.org/releases/mozilla-esr10/rev/f78a0fd268cf
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
Whiteboard: STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?
Whiteboard: [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+][qa+] STR in comment #10, maybe OS X 10.7 only?
I'm willing to work on the in-testsuite request if someone can provide me some mentorship.
QA Contact: anthony.s.hughes
Keywords: verifyme
Confirmed testcase crashes 2012-06-12 Firefox 16.0a1 Nightly Debug.

Does not crash:
2012-08-02 Firefox 17.0a1 Nightly Debug
2012-08-04 Firefox 16.0a2 Aurora Debug
2012-08-04 Firefox 15.0 Beta Debug
2012-08-04 Firefox 10.0.7 ESR Debug
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
Keywords: verifyme
Whiteboard: [advisory-tracking+][qa+] STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?
Group: core-security
You need to log in before you can comment on or make changes to this bug.