601699 - Crash [@ nsSVGPathElement::GetMarkPoints]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Scary: "Crash address: 0x29bdbf0"

Comment 2

[Damon Sicore (:damons)]

Assigning to jwatt.  Jonathan, can you take a look?

Comment 3

[Jonathan Watt [:jwatt]]

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?

Comment 4

[Jesse Ruderman]

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?)

Comment 5

[Jesse Ruderman]

> (Two mRefCnt members? How the hell?)

Harmless, see bug 602122.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 7

[Robert Longson [:longsonr]]

Looks like this fixes bug 366130 too.

Comment 8

[Daniel Veditz [:dveditz]]

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

Comment 9

[[e-mail] (:bhearsum)]

This landed in the wrong place on 1.9.2; on a really old relbranch (GECKO1924_20100413_RELBRANCH) instead of default.

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Jonathan Watt [:jwatt]]

(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.

Comment 12

[Jonathan Watt [:jwatt]]

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

Comment 13

[Al Billings [:abillings - ex-MoCo]]

(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?

Comment 14

[Jonathan Watt [:jwatt]]

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.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

That points to the necessity of a debug build then (as opposed to an actual release build which is optimized).

Comment 16

[Jonathan Watt [:jwatt]]

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.