Canvas2D crash [@mozilla::gfx::DrawTargetCG::StrokeRect]

RESOLVED FIXED in Firefox 25, Firefox OS v1.2

Status

()

Core
Canvas: 2D
--
critical
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: posidron, Assigned: milan)

Tracking

(Blocks: 2 bugs, {crash, sec-critical, testcase})

Trunk
mozilla29
x86_64
Mac OS X
crash, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox23 wontfix, firefox24+ wontfix, firefox25+ verified, firefox26+ verified, firefox27 verified, firefox28 fixed, firefox29 fixed, firefox-esr1725+ verified, firefox-esr2425+ verified, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [Fix moved to bug 913614][adv-main25+][adv-est1710+][esr-24-1+])

Attachments

(9 attachments, 4 obsolete attachments)

216 bytes, text/html
Details
3.87 KB, text/plain
Details
214 bytes, text/html
Details
303 bytes, text/html
Details
1.23 KB, patch
jrmuizel
: review-
Details | Diff | Splinter Review
389 bytes, text/html
Details
5.46 KB, patch
milan
: review+
[PTO until July 27]
: sec-approval+
Details | Diff | Splinter Review
16.85 KB, patch
milan
: review+
Details | Diff | Splinter Review
5.95 KB, patch
milan
: review+
[PTO until July 27]
: approval-mozilla-aurora+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 775365 [details]
testcase

if (isGradient(aPattern)) {
    // There's no CGContextClipStrokeRect so we do it by hand
    CGContextBeginPath(cg);
    CGContextAddRect(cg, RectToCGRect(aRect));
    CGContextReplacePathWithStrokedPath(cg);
    //XXX: should we use EO clip here?
    CGContextClip(cg);
    DrawGradient(cg, aPattern);
  } else {
    SetStrokeFromPattern(cg, mColorSpace, aPattern);
*   CGContextStrokeRect(cg, RectToCGRect(aRect));
  }



Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/fa6ef0b63025
(Reporter)

Comment 1

5 years ago
Created attachment 775366 [details]
callstack

Comment 2

5 years ago
Milan, who can we assign this to?
Flags: needinfo?(milan)
Assignee: nobody → jmuizelaar
What Joe said.
Blocks: 891660
Flags: needinfo?(milan)
I believe this stopped crashing for me under ASAN in the nightly.  I don't have a window as to when it stopped, probably within the past couple of weeks.

Comment 6

5 years ago
Jeff, any updates here?
status-firefox23: --- → ?
status-firefox24: --- → ?
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox-esr17: --- → ?
tracking-firefox26: --- → +
Thought this was only crashing behind a pref, but now it does crash on the default profile.  The reason it "worked" for me was because I had switched to SkiaGL instead.  That one runs clean.
Right - there is a crash with the default configuration.  Setting gfx.canvas.azure.backends from the default cg to skia stops the crash.  If left at the default cg, we crash.  gfx.canvas.azure.accelerated setting is irrelevant - crashes with the default false and non-default true.  Yes, it's obvious from the original comment, but thought I'd mention it because I got caught on the pref change.
Created attachment 788381 [details]
This testcase is OK - fillRect seems to be OK.

This test should pass, with fillRect instead of strokeRect.
Created attachment 788382 [details]
fillRect + clearRect - also OK

This test is also OK - fillRect + clearRect
Created attachment 788385 [details] [diff] [review]
893572.p

To build, this needs a patch from bug 903526 - the feedback is to see if it makes sense to just reject degenerate rectangles, and also whether to just do it in StrokeRect (the crash here) or also in FillRect and/or ClearRect even though they have no ASAN errors with the infinite values.
Attachment #788385 - Flags: feedback?(jmuizelaar)
Al, a question - the patch for this looks like a performance improvement.  However, attaching it to a security bug, one can "see" how to trigger this crash (I don't know about exploiting it, though). Does it make sense to open a different (performance?) bug and make the fix against that one, or just do it in this one?
Flags: needinfo?(abillings)
Created attachment 790301 [details] [diff] [review]
Guard StrokeRect on OSX against "infinite" values

Just guarding StrokeRect - should the same guards be done in ClearRect and FillRect (no crash in those, it seems.)
Attachment #788385 - Attachment is obsolete: true
Attachment #788385 - Flags: feedback?(jmuizelaar)
Attachment #790301 - Flags: review?(jmuizelaar)

Comment 14

5 years ago
I would open the perf cover bug.

That said, before this is checked in anywhere, you should set sec-approval? on the bug and fill out the template.

We don't really know how far back this goes?
Flags: needinfo?(abillings)
I believe it's been around since January '12 when the Azure on OSX got enabled.  However, this is a crash in CoreGraphics itself, we are just working around it, and I don't know if that crash exists in all versions of OSX.  I'll get the patch, get the sec-approval, and we can then decide if it needs an uplift.
status-b2g18: --- → wontfix
status-firefox23: ? → wontfix
status-firefox24: ? → affected
status-firefox-esr17: ? → affected
Comment on attachment 790301 [details] [diff] [review]
Guard StrokeRect on OSX against "infinite" values

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

I wonder if this guard should be moved up to canvas? Could we have the same problem if the rectangle is constructed manually with a path.
Also this patch should include the test as a crash-test reftest.
Attachment #790301 - Flags: review?(jmuizelaar) → review-
Anything that calls CGContextStrokeRect will have a problem, but that's only the rect.  I believe you're asking if CGContextStrokePath would also have a problem - I will check and get the test done.
Assignee: jmuizelaar → milan
Path with "infinite" seems to be OK.  I'll add it to the same crash test.
Created attachment 791467 [details]
893572-testcase-line.html

I lied - it does crash with the path as well.  Here's the test.
Created attachment 792946 [details] [diff] [review]
Catch the non-finite numbers on OS X that crash in the CG code.

Check for inf in paths, lines or rectangles guarding against crashes on OS X. Some of these methods are being called from multiple places, are public methods, so it seems we need to protect inside DrawTargetCG, rather than up in the canvas code, in case they get used elsewhere. Note this is only an issue for numbers larger than float max and smaller than double max, because those will get through the LenientFloat in the IDL.
Attachment #792946 - Flags: review?(jmuizelaar)
Speaking of LenientFloat and IDL - that's bug 767933.
Review from jmuizelaar@mozilla.com?
Flags: needinfo?(jmuizelaar)

Updated

5 years ago
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox-esr17: --- → ?
I am tracking this because its a sec-crit but given where we are in the cycle for Fx24 and keeping in mind this bug is old enough and dates back to January '12, this will be a wontfix unless anyone can make a strong case to take it in our final two weeks of Beta and deems the patch is super safe to get it landed by Monday, which is when our next beta is going to build.
I think wontfix for the Beta is reasonable.  I'm 50/50 on whether we should put it in Aurora, I think we could decide to just let it ride the 26 train.  Having said that, how does this bug date back to January '12?
(In reply to Milan Sreckovic [:milan] from comment #24)
> I think wontfix for the Beta is reasonable. 
wontfix'ing for fx24.
>Got i from your comment  I'm 50/50 on whether we should
> put it in Aurora, I think we could decide to just let it ride the 26 train. 
> Having said that, how does this bug date back to January '12?

Got it from your comment above https://bugzilla.mozilla.org/show_bug.cgi?id=893572#c15, did I catch something wrong ?

Updated

5 years ago
status-firefox24: affected → wontfix
tracking-firefox24: ? → +
tracking-firefox25: ? → +
(In reply to bhavana bajaj [:bajaj] from comment #25)
> (In reply to Milan Sreckovic [:milan] from comment #24)
> > I think wontfix for the Beta is reasonable. 
> wontfix'ing for fx24.
> >Got i from your comment  I'm 50/50 on whether we should
> > put it in Aurora, I think we could decide to just let it ride the 26 train. 
> > Having said that, how does this bug date back to January '12?
> 
> Got it from your comment above
> https://bugzilla.mozilla.org/show_bug.cgi?id=893572#c15, did I catch
> something wrong ?

Fair enough, it probably has been around for that long.
Attachment #792946 - Flags: review?(jmuizelaar) → review+
Comment on attachment 792946 [details] [diff] [review]
Catch the non-finite numbers on OS X that crash in the CG code.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Fairly easily to crash, not sure if it can be exploited.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
If attached to the security bug, probably.

Which older supported branches are affected by this flaw?
This is a problem in CoreGraphics, so most of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
If we want to, it shouldn't be difficult to port.

How likely is this patch to cause regressions; how much testing does it need?
Low chance of regressions.  The patch changes the behaviour of the code that would have crashed before. Should not have impact on "normal" data.
Attachment #792946 - Flags: sec-approval?
Flags: needinfo?(jmuizelaar)
status-b2g18: wontfix → unaffected
tracking-firefox-esr17: ? → 25+
Depends on: 903526
Comment on attachment 792946 [details] [diff] [review]
Catch the non-finite numbers on OS X that crash in the CG code.

We can't check this in with tests, the tests will have to land after the fix ships. In fact we really ought to tell Apple about the CoreGraphics bug before checking in the tests so we might want to clone a separate "check in the tests" bug.

This also apparently needs the patch from bug 903526 so the ESR-17 and Aurora backports will have to contain that code as well (or if it applies as is then a branch-approval request in that bug).

Waiting for the roll-up branch patches before granting approval so we know they're ready to go in together (and also because I'm nervous about approving an hg bundle with tests in case someone else checks in without reading the comments).
Right - see also Comment 14; I was assuming we would spin up a separate bug, call it "performance" so that we don't draw attention to this being a security bug.  The fix would go in there, the tests would eventually (when?) get landed with this bug.  Or something like that?
I could approve that approach, yes
This "fake" bug 913614.  Please don't set dependencies between these two bugs.  Daniel, I will let you know when the fix only (same fix extracted from the patch above) is attached to bug 913614 and you can approve it in this bug before we get it checked in.
Created attachment 800926 [details] [diff] [review]
Additional crashtests to catch this problem.  The code fix is part of a bug 913614.  Carry r=jmuizelaar.

This is the tests only part of the attachment 792946 [details] [diff] [review] as the code part is in bug 913614.

The security approval flag should only be set once we want these tests returned.
Attachment #792946 - Attachment is obsolete: true
Attachment #792946 - Flags: sec-approval?
Attachment #800926 - Flags: sec-approval?
Attachment #800926 - Flags: review+
(In reply to Daniel Veditz [:dveditz] from comment #30)
> I could approve that approach, yes

There is now an attachment in bug 913614 with the code changes alone.  Daniel, please let me know in this bug if that attachment to bug 913614 is OK to land on central.
Flags: needinfo?(dveditz)
sec-approval+ on the patch in bug 913614
Flags: needinfo?(dveditz) → in-testsuite?
Whiteboard: Don't check in tests until after Fx 25 release. Fix moved to bug 913614.
Bhavana, look for the aurora approval in bug 913614 - it's really for this bug, but we don't want to mention the existence of this bug in 913614.
Flags: needinfo?(bbajaj)
(In reply to Milan Sreckovic [:milan] from comment #37)
> Bhavana, look for the aurora approval in bug 913614 - it's really for this
> bug, but we don't want to mention the existence of this bug in 913614.

Just approved ! Also setting flags for esr24 as the patch would be needed on that branch as well.
Flags: needinfo?(bbajaj)

Updated

5 years ago
status-firefox-esr24: --- → affected
tracking-firefox-esr24: --- → 25+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5706a9c6704a
status-firefox25: affected → fixed
status-firefox26: affected → fixed
Target Milestone: --- → mozilla26
Backed out for bustage due to dependencies not being nommed and approved for uplift...
https://hg.mozilla.org/releases/mozilla-aurora/rev/97e0d6a03696
status-firefox25: fixed → affected
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> Backed out for bustage due to dependencies not being nommed and approved for
> uplift...
> https://hg.mozilla.org/releases/mozilla-aurora/rev/97e0d6a03696

I don't think this one is meant to be checked in at all until 25 ships?  See the whiteboard.  Once 25 ships, 26 will be in beta, and we will then check this into 28 and uplift to 27 aurora and 26 beta.  At least, this is how I understood it, and this is why sec-approval is still a "?"
Looks like bug 913614 landed in 25.
status-firefox25: affected → fixed
Marking fixed because it is fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
This still needs to be landed on ESR-17 and ESR-24, it looks like.  (If we're still landing things on 17...)
(In reply to Andrew McCreight [:mccr8] from comment #44)
> Marking fixed because it is fixed on trunk.

Not completely - the tests are not checked in anywhere.  We're waiting for the fix to go public with 25, before we return the tests.  So, there still needs to be something that happens to this bug - perhaps we can leave it open so that we don't lose track of it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sure.
status-firefox27: --- → fixed

Comment 48

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #45)
> This still needs to be landed on ESR-17 and ESR-24, it looks like.  (If
> we're still landing things on 17...)

We're still supporting both (there is a two version overlap) so we should take it on both.
Confirmed crash FF25, 2013-07-11.
Verified fixed FF25, FF26, FF27, 2013-10-01.
status-firefox25: fixed → verified
status-firefox26: fixed → verified
status-firefox27: fixed → verified

Comment 50

5 years ago
Comment on attachment 800926 [details] [diff] [review]
Additional crashtests to catch this problem.  The code fix is part of a bug 913614.  Carry r=jmuizelaar.

Setting sec-approval+ per comment 34 but tests shouldn't land until after 10/29 when Firefox 25 ships.
Attachment #800926 - Flags: sec-approval? → sec-approval+

Updated

5 years ago
Whiteboard: Don't check in tests until after Fx 25 release. Fix moved to bug 913614. → Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29]
Hi Milan, can someone land this fix (from bug 913614) on ESR? (Looking at the flags I assume that didn't happen yet).
Flags: needinfo?(milan)
(In reply to David Bolter [:davidb] from comment #51)
> Hi Milan, can someone land this fix (from bug 913614) on ESR? (Looking at
> the flags I assume that didn't happen yet).

We could put it together, but I was thinking that comment 15 and comment 25 were having us skip the 24ESR.  Do we want it there?
Flags: needinfo?(milan)
I guess we do because landing the tests in 25 gives people a hint how to break 24?

Comment 54

5 years ago
ESR24 should take important security fixes for Firefox 25 (and 26, 27, etc. until end of life). 

This was sec-critical, right?
Created attachment 816090 [details] [diff] [review]
24 ESR Patch for this and the dependent bugs.  Carry r=jmuizelaar.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It is sec-critical.

User impact if declined: 
SEcurity vulnerability 

Fix Landed on Version: 25

Risk to taking this patch (and alternatives if risky): 
After 25 ships, we will be landing a test that reproduces this crash on 24.

String or UUID changes made by this patch: None
Attachment #816090 - Flags: review+
Attachment #816090 - Flags: approval-mozilla-esr24?
(In reply to Al Billings [:abillings] from comment #54)
> ESR24 should take important security fixes for Firefox 25 (and 26, 27, etc.
> until end of life). 
> 
> This was sec-critical, right?

I attached the 24 ESR patch - it should wait until after 25 ships, as it contains the same set of tests that expose the vulnerability.

Comment 57

5 years ago
The whole point was to ship the fix in ESR24 at the same time as we ship Firefox 25 though. Landing this after 25 ships kind of defeats the point of not zero daying ESR24 with a sec-critical. I'm less concerned about the tests than making sure the underlying vuln is fixed in ESR24.
You're right, I should have said "not before 25 ships", so at the same time would work perfectly!

Updated

5 years ago
Attachment #816090 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+

Updated

5 years ago
Whiteboard: Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29] → Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29][adv-main25+]

Comment 59

5 years ago
Let's get this checked into ESR24 ASAP.

Comment 60

5 years ago
We seem to have missed the windows for ESR24?
Flags: needinfo?(milan)

Comment 61

5 years ago
And ESR17 as well.

Any comments from release management?
Flags: needinfo?(release-mgmt)

Comment 62

5 years ago
Release Management says in IRC that you can land this for ESR17 and ESR24 if you land it today, Milan.
Flags: needinfo?(release-mgmt)
I'm currently cloning ESR24 for landing the patch https://bugzilla.mozilla.org/attachment.cgi?id=816090

It sounds like I should land it without the tests, so I'll just land the code changes.

Is it expected to cleanly apply to ESR17?
I don't know if this will work or not, but at least it applies cleanly, which gives me hope.

https://hg.mozilla.org/releases/mozilla-esr17/rev/eeab87c4c2db

(I landed without patches, and with the same checkin message as was used previously for bug 913614)
status-firefox-esr17: affected → fixed

Comment 66

5 years ago
Thanks for landing these.
Thanks indeed.  I'm in CST at a work week (this week), so I didn't see this exchange until now.
Flags: needinfo?(milan)

Updated

5 years ago
Whiteboard: Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29][adv-main25+] → Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29][adv-main25+][adv-est1710+][esr-24-1+]

Updated

5 years ago
status-firefox28: --- → unaffected
Verified on shipping 17.0.10esr and 24.1esr.
status-firefox-esr17: fixed → verified
status-firefox-esr24: fixed → verified

Comment 69

5 years ago
It is after 10/29. Can we land tests?
(In reply to Al Billings [:abillings] from comment #69)
> It is after 10/29. Can we land tests?

Yes please!
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb843a74b5b
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: Don't check in tests until after Fx 25 release. Fix moved to bug 913614. [land tests after 10/29][adv-main25+][adv-est1710+][esr-24-1+] → [Fix moved to bug 913614][adv-main25+][adv-est1710+][esr-24-1+]
The tests crash... this sadly isn't fixed after all :-s

https://tbpl.mozilla.org/php/getParsedLog.php?id=30334956&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30334722&tree=Mozilla-Inbound

{
09:15:43  WARNING -  PROCESS-CRASH | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/gfx/tests/crashtests/893572-2.html | application crashed [@ CoreGraphics + 0x188b9b]
09:15:43     INFO -  Crash dump filename: /var/folders/gc/nk_nqbn10l9_5k6rmxmkt_9400000w/T/tmpj_RO4Y.mozrunner/minidumps/94AB241A-5616-4AA8-BC52-D541227F2D2D.dmp
09:15:43     INFO -  Operating system: Mac OS X
09:15:43     INFO -                    10.7.2 11C74
09:15:43     INFO -  CPU: amd64
09:15:43     INFO -       family 6 model 23 stepping 10
09:15:43     INFO -       2 CPUs
09:15:43     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
09:15:43     INFO -  Crash address: 0x5fc39d1d
09:15:43     INFO -  Thread 0 (crashed)
09:15:43     INFO -   0  CoreGraphics + 0x188b9b
09:15:43     INFO -      rbx = 0x0000000000000125   r12 = 0x0000000000000000
09:15:43     INFO -      r13 = 0x000000013c80a128   r14 = 0x0000000000000001
09:15:43     INFO -      r15 = 0x00007fff5fc39d1d   rip = 0x00007fff8de55b9b
09:15:43     INFO -      rsp = 0x00007fff5fbf9c80   rbp = 0x00007fff5fbf9cc0
09:15:43     INFO -      Found by: given as instruction pointer in context
09:15:43     INFO -   1  CoreGraphics + 0x188701
09:15:43     INFO -      rip = 0x00007fff8de55702   rsp = 0x00007fff5fbf9cd0
09:15:43     INFO -      rbp = 0x00007fff5fbf9d40
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   2  CoreGraphics + 0x1882d4
09:15:43     INFO -      rip = 0x00007fff8de552d5   rsp = 0x00007fff5fbf9d50
09:15:43     INFO -      rbp = 0x00007fff5fbf9d80
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   3  CoreGraphics + 0x187e4b
09:15:43     INFO -      rip = 0x00007fff8de54e4c   rsp = 0x00007fff5fbf9d90
09:15:43     INFO -      rbp = 0x00007fff5fbf9e10
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   4  libRIP.A.dylib + 0x5f1e
09:15:43     INFO -      rip = 0x00007fff8e7e0f1f   rsp = 0x00007fff5fbf9e20
09:15:43     INFO -      rbp = 0x00007fff5fbf9ed0
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   5  libRIP.A.dylib + 0x573c
09:15:43     INFO -      rip = 0x00007fff8e7e073d   rsp = 0x00007fff5fbf9ee0
09:15:43     INFO -      rbp = 0x00007fff5fbf9f50
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   6  libRIP.A.dylib + 0x8b4a
09:15:43     INFO -      rip = 0x00007fff8e7e3b4b   rsp = 0x00007fff5fbf9f60
09:15:43     INFO -      rbp = 0x00007fff5fbfa080
09:15:43     INFO -      Found by: stack scanning
}

Backed the tests out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a92a46ff6be
Flags: needinfo?(milan)
Let me check.  There were recent changes to the original fix, may have caused this problem.
Flags: needinfo?(milan)
The tests pass in release, beta and aurora, but fail on central.  I'll find a regression range.
The regression comes from bug 922942 - https://bug922942.bugzilla.mozilla.org/attachment.cgi?id=826199
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bas)
On a side note, BaseRect::IsFinite() as of recently works on windows.
Flags: needinfo?(bas)
Are you sure it's that change?

The change was mainly to gfxContext, which shouldn't be used by canvas at all.

The only other change was adding two IsFinite checks to DrawTargetCG, and it looks like canvas only calls those functions if we have shadows, or are drawing a focus ring.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #78)
> Are you sure it's that change?
> 
> The change was mainly to gfxContext, which shouldn't be used by canvas at
> all.
> 
> The only other change was adding two IsFinite checks to DrawTargetCG, and it
> looks like canvas only calls those functions if we have shadows, or are
> drawing a focus ring.

I agree this seems extremely unlikely..
(In reply to Bas Schouten (:bas.schouten) from comment #79)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #78)
> > Are you sure it's that change?
> > ...
> 
> I agree this seems extremely unlikely..

And yet... the crash goes away if you back out this change from PathCG::GetBounds function :)

+  if (!bounds.IsFinite()) {
+    return Rect();
+  }

The comment above says:
//XXX: what should these functions return for an empty path?
// currently they return CGRectNull {inf,inf, 0, 0}

except that with the above change, we start returning (0,0,0,0) instead.  Seems that somewhere down the line, having a (0,0,0,0) rectangle for that path that contains infinite causes problems.

Doing this:

if (!bounds.IsFinite()) {
  return bounds;
}

actually makes this particular crash go away.  Matt, what do you think?  Is that a reasonable change?
Flags: needinfo?(matt.woodrow)
Same question for D2D.  We return 0 sized bounds for a path that contains infinity, which seems dangerous.
Flags: needinfo?(bas)
If it passes all existing tests, then it seems fine.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #82)
> If it passes all existing tests, then it seems fine.

Nope, it regresses test_text_selection.html.  I'll look at this a bit, but may need to pass it back.
Flags: needinfo?(bas)
Clearing the flag as this is being looked into :)
Created attachment 8337770 [details] [diff] [review]
tmp

I wonder if this patch will have more luck.
Attachment #8337770 - Flags: feedback?(milan)
Attachment #8337770 - Flags: feedback?(matt.woodrow)
Comment on attachment 8337770 [details] [diff] [review]
tmp

F+ meaning that this fixes the crash for me.  Pushed to try even as this is security bug, as all that's left are the tests.
https://tbpl.mozilla.org/?tree=Try&rev=ee2916dc1589
Attachment #8337770 - Flags: feedback?(milan) → feedback+
Looks like this doesn't pass tests either :(
Not sure about that.  The test_text_selection has had trouble passing last time I did a try run and were failing for me locally with or without this change.  Haven't looked at the rest.
But it must be passing on the tbpl machines without this patch, since we're definitely green on m-c.
Indeed.  All I'm saying is that the same test fails for me locally without the patch.
Or maybe not. Never mind.

Without the change, the new crashtests from this bug fail.
With the change from comment 85, the new crashtests from this bug succeed.  The mochitest 1 ( test_text_selection.html) fails.
Attachment #8337770 - Flags: feedback?(matt.woodrow)
Attachment #8337770 - Flags: feedback-
Attachment #8337770 - Flags: feedback+
The problem is rounding inf/nan rectangles into integer ones for SVG to use.
Created attachment 8338677 [details] [diff] [review]
Watch when converting float to int rectangles and infinites.

This is just putting the check in the method that was causing the tests to fail.
This backs out part of the bug 922942 that was meant to get the SVG tests to pass.
We may want to put warnings or asserts in BaseRect::Round* methods to notify us of non-finite scenarios, and we may even want to put the checks in there - seems like we may as well wait with the checks, see how common this is.

https://tbpl.mozilla.org/?tree=Try&rev=71145173d1fd
Attachment #8337770 - Attachment is obsolete: true
Attachment #8338677 - Flags: review?(matt.woodrow)
Attachment #8338677 - Flags: review?(bas)
Comment on attachment 8338677 [details] [diff] [review]
Watch when converting float to int rectangles and infinites.

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

::: gfx/2d/PathCG.cpp
@@ +289,5 @@
>  {
>    //XXX: are these bounds tight enough
>    Rect bounds = CGRectToRect(CGPathGetBoundingBox(mPath));
>    if (!bounds.IsFinite()) {
> +    return bounds;

Technically I wonder if we should still transform here, since [x,inf] can still be translated to [x+a,inf] validly. Also I'm wondering whether we should still watch for NaN or not.
Attachment #8338677 - Flags: review?(bas) → review+
Attachment #8338677 - Flags: review?(matt.woodrow) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #94)
> Comment on attachment 8338677 [details] [diff] [review]
> Watch when converting float to int rectangles and infinites.
> 
> Review of attachment 8338677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/PathCG.cpp
> @@ +289,5 @@
> >  {
> >    //XXX: are these bounds tight enough
> >    Rect bounds = CGRectToRect(CGPathGetBoundingBox(mPath));
> >    if (!bounds.IsFinite()) {
> > +    return bounds;
> 
> Technically I wonder if we should still transform here, since [x,inf] can
> still be translated to [x+a,inf] validly. Also I'm wondering whether we
> should still watch for NaN or not.

On the NaN side - are you saying we should only watch for NaN, or not watch for NaN at all, and just watch for inf?

On the transform side - I could leave the transform in there; in the test case, we start with (inf,inf,0,0), which, after the transform, ends up (inf,inf,inf,inf).
Keywords: checkin-needed
Comment 95 makes it seem like this isn't ready to land yet? Also, the Try push in comment 93 is showing OSX reftest failures.
Keywords: checkin-needed

Comment 97

5 years ago
Any updates here, Milan?
Flags: needinfo?(milan)
Not a lot.  The try fails, but locally things work (on 10.8 debug), so it's a bit more of a pain to figure out what's going on.
Flags: needinfo?(milan)
Created attachment 8349026 [details] [diff] [review]
The tests that catch the problem.  Backout part of 922942.  Carry r=bas, mattwoodrow

Address comment #95, add the tests, back out part of 922942 that caused the tests to regress.  https://tbpl.mozilla.org/?tree=Try&rev=de1dbbc4e939
Attachment #8338677 - Attachment is obsolete: true
Attachment #8349026 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3735493e9aa8

Pretty sure we're going to need this on Aurora as well given that bug 922942 landed on Fx28.
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → fixed
status-b2g-v1.3: --- → affected
status-firefox28: unaffected → affected
status-firefox29: --- → affected
Keywords: checkin-needed
Comment on attachment 8349026 [details] [diff] [review]
The tests that catch the problem.  Backout part of 922942.  Carry r=bas, mattwoodrow

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Combination of the original problem and bug 922942
User impact if declined: 
We have part of the original bug broken by 922942 - security risk.
Testing completed (on m-c, etc.): m-c.
Risk to taking this patch (and alternatives if risky): 
sec-critical issue.
String or IDL/UUID changes made by this patch: n/a
Attachment #8349026 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #8349026 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/3735493e9aa8
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox29: affected → fixed
Resolution: --- → FIXED
Target Milestone: mozilla26 → mozilla29
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd169dd60c78

I plan to push the test to beta/b2g26/esr24 as a ride-along as well.
status-b2g-v1.3: affected → fixed
status-firefox28: affected → fixed
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
Depends on: 1121835
Group: core-security
(Reporter)

Updated

2 years ago
Blocks: 1289929
You need to log in before you can comment on or make changes to this bug.