Crash [@ nsSVGPathElement::GetMarkPoints]

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
x86
Mac OS X
crash, testcase
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .13+, status1.9.2 .13-fixed, blocking1.9.1 .16+, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:critical?] [qa-needs-STR], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Created attachment 480696 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

7 years ago
Created attachment 480697 [details]
stack trace

Scary: "Crash address: 0x29bdbf0"
Assigning to jwatt.  Jonathan, can you take a look?
Assignee: nobody → jwatt
blocking2.0: --- → final+
(Assignee)

Comment 3

7 years ago
As it happens, when I was discussing another bug with jst earlier today, he noticed that we have two mRefCnt members for path segments. It may not be related, but it's highly suspicious.

Anyway, I'm completely rewriting this code in bug 522306, so it would make most sense to check if we still have this problem after I'm done with that. Does that sound okay?
(Reporter)

Comment 4

7 years ago
I think this also affects branches such as 1.9.2, so it would be nice to have a more direct fix.  I can't reproduce the crash at the moment but I do see an invalid array index assertion on both trunk and 1.9.2.

(Two mRefCnt members? How the hell?)
(Reporter)

Comment 5

7 years ago
> (Two mRefCnt members? How the hell?)

Harmless, see bug 602122.
(Assignee)

Comment 6

7 years ago
Created attachment 481187 [details] [diff] [review]
patch
Attachment #481187 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #481187 - Flags: review? → review?(longsonr)

Updated

7 years ago
Attachment #481187 - Flags: review?(longsonr) → review+

Comment 7

7 years ago
Looks like this fixes bug 366130 too.
(Assignee)

Updated

7 years ago
Blocks: 366130
(Assignee)

Updated

7 years ago
Attachment #481187 - Flags: approval1.9.2.12?
Attachment #481187 - Flags: approval1.9.1.15?
Comment on attachment 481187 [details] [diff] [review]
patch

>+      // first non-moveto segment after a moveto
>       pathIndex = aMarks->Length() - 1;
>       pathAngle = startAngle;
>       aMarks->ElementAt(pathIndex).angle = pathAngle;

So the comment is saying that after a moveto you're guaranteed aMarks->Length() isn't 0? Otherwise I don't see why this doesn't have the same problem you're fixing a few lines down.

Approved for 1.9.2.12 and 1.9.1.15, a=dveditz for release-drivers
Attachment #481187 - Flags: approval1.9.2.12?
Attachment #481187 - Flags: approval1.9.2.12+
Attachment #481187 - Flags: approval1.9.1.15?
Attachment #481187 - Flags: approval1.9.1.15+
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
status1.9.1: --- → wanted
status1.9.2: --- → wanted
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
This landed in the wrong place on 1.9.2; on a really old relbranch (GECKO1924_20100413_RELBRANCH) instead of default.
The broken 1.9.2 landing noted in comment 9 was:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6b4d4bdf28fe

Looks like jwatt landed it on 1.9.1 as well (correctly there -- not on a relbranch):  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b55da5097c00
Keywords: checkin-needed
(Assignee)

Comment 11

7 years ago
(In reply to comment #8)
> So the comment is saying that after a moveto you're guaranteed aMarks->Length()
> isn't 0? Otherwise I don't see why this doesn't have the same problem you're
> fixing a few lines down.

The problem is that we were running the code |aMarks->ElementAt(aMarks->Length() - 1)| when the length of aMarks was zero, causing us to access memory at a bad location. That's what the new check for |aMarks->Length()| prevents (makes sure the length of the nsTArray is at least 1).

The comment, to answer your question, says that we know that we already have an item in aMarks, so there's no need for a |aMarks->Length()| check there.
(Assignee)

Comment 12

7 years ago
1.9.1 commit:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b55da5097c00

1.9.2 commit:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1fbb00c824a9

On mozilla-central the code in question was substantially rewritten by the patch for bug 522306. That rewrite also fixed this bug, so on mozilla-central I just committed a test to make sure we don't regress:

http://hg.mozilla.org/mozilla-central/rev/981ba35eb1c3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
No longer blocks: 366130

Updated

7 years ago
status1.9.1: wanted → .16-fixed
status1.9.2: wanted → .13-fixed
(In reply to comment #0)
> Created attachment 480696 [details]
> testcase (crashes Firefox when loaded)

This might crash trunk but it doesn't crash 1.9.2.12 when loaded on OS X 10.6.5 in the release build. Was this crash seen with normal 1.9.2 builds?
Whiteboard: [sg:critical?] → [sg:critical?] [qa-needs-STR]
(Assignee)

Comment 14

7 years ago
It wasn't a very reliable crash on trunk, and apparently not on 1.9.2 either. It did reliably hit the assertion saying the index was bad though. See comment 4.
That points to the necessity of a debug build then (as opposed to an actual release build which is optimized).
(Assignee)

Comment 16

7 years ago
To verify the fix? Yes, I guess so. But this bug is very much present in optimized builds too - the buggy logic and inevitability of going down the bad code path is the same in both.
Group: core-security
Crash Signature: [@ nsSVGPathElement::GetMarkPoints]
You need to log in before you can comment on or make changes to this bug.