664930 - Crash [@ _cairo_bentley_ottmann_tessellate_rectangular]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Possibly a regression from the recent cairo update?
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=102be3d1f103

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

Security-sensitive because it often crashes trying to access 0x1e7c2f8 rather than something near 0x0.

Comment 3

[Johnny Stenback (:jst)]

Jeff, this sounds like something that came in with your most recent cairo import.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

    pos->prev->next = edge;

pos->prev is bogus. Not sure why yet. It looks like this is caused by an overflow of some value.

Comment 5

[Daniel Veditz [:dveditz]]

Does this affect Aurora? if it's a regression from the changeset in comment 0 then it should be in Aurora, but we need to test

Comment 6

[Johnny Stenback (:jst)]

Jeff, any updates here? It's getting late for 6, but if we can get a safe fix for 6 soon we'd still consider it.

Comment 7

[Johnny Stenback (:jst)]

No updates so I don't think we can take this for 6 any more.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

This shouldn't affect 6.

Comment 9

[Johnny Stenback (:jst)]

Jeff, any progress here? This has seemingly been sitting untouched for a while...

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Standalone cairo test case that reproduces the problem

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Handle the case where an edge lies at the left most coordinate

This patch fixes the issue. The issue also appears to be just a NULL deference and so wouldn't be exploitable.

Comment 12

[Johnny Stenback (:jst)]

Removing sg:critical severity then to trigger a re-triage, with the likely outcome being that we'll open this bug up.

Comment 13

[Jesse Ruderman]

See comment 2 for why I initially marked the bug as sg:critical ...

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #13)
> See comment 2 for why I initially marked the bug as sg:critical ...

Hmm... that's interesting. I didn't see how that could happen with the thing that I fixed. It might be worth waiting a bit before opening this up.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

I took a closer look at this. The NULL dereference is not actually a NULL it's uninitialized stack memory so this is more severe than I originally thought. It's not clear if the unintialized value is controllable by content but we might as well assume that it is.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Handle the case where an edge lies at the left most coordinate and initialize the rest of the data

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 551582 [details] [diff] [review]
Handle the case where an edge lies at the left most coordinate and initialize the rest of the data

r=me with the following change:

>+    /* we need to initialize prev so that we can check
>+     * if this edge is the left most and make sure
>+     * we always insert to the right of it, even if
>+     * our x coordinate matches */
>+    sweep_line->head.prev = NULL;

Please initialize head.next = NULL too...

>-
>-    pos->prev->next = edge;
>-    edge->prev = pos->prev;
>-    edge->next = pos;
>-    pos->prev = edge;
>+    if (pos->prev) {
>+        pos->prev->next = edge;
>+        edge->prev = pos->prev;
>+        edge->next = pos;
>+        pos->prev = edge;
>+    } else {
>+        /* we have edge that shares an x coordinate with the left most sentinal.
>+         * instead of inserting before pos and ruining our sentinal we insert after pos. */
>+        pos->next->prev = edge;

So that if there ever is a weird case (zero-length list?) where there wouldn't be a pos->next here, at least we'd get a good null deref instead of an uninitialized pointer deref.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Attachment: Handle the case where an edge lies at the left most coordinate and initialize the rest of the data v2

Adjusts to match review comments.

Comment 19

[(no longer active)]

Landed on inbound.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/1fc015db1f6f

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 552727 [details] [diff] [review]
Handle the case where an edge lies at the left most coordinate and initialize the rest of the data v2

Aurora [7] window has closed. Moving approval request to Beta.

Comment 22

[LegNeato]

Comment on attachment 552727 [details] [diff] [review]
Handle the case where an edge lies at the left most coordinate and initialize the rest of the data v2

Approved for mozilla-beta

Comment 23

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-beta/rev/2347184b3d96

Comment 24

[u279076]

qa+ for verification with Firefox 7.

Comment 25

[u279076]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110926 Firefox/9.0a1 ID:20110926030901

Using the attached testcase I am unable to reproduce this crash. Marking verified.