360293 - passing in non-numeric values to canvas functions causes crash inside cairo

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The Canvas API uses floats in the IDL, so the C++ method signatures just accept floats.  xpconnect uses JS_ValueToNumber to do the conversion from jsval to double, and ValueToNumber happily coverts non-numeric values (e.g. "foo") to NaN.  Cairo doesn't handle NaN (or, I assume, +/-Inf) well, and so it leads to a crash.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: testcase (crashes once you click crash button)

Testcase

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not sure if this is exploitable -- we should backport to 1.8.0.9 if so (trivial backport), otherwise we should just get it in to 1.8.1.x.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add float value checks to canvas

Add checks to make sure each float value passed in is finite; error out otherwise.

Comment 4

[Daniel Veditz [:dveditz]]

In a windows debug build it crashes when we pass an invalid pointer to realloc, that's potentially bad.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixed patch using JSDOUBLE_IS_FINITE. patch for 1.8.1/1.9.

Better patch, using jsnum.h and JSDOUBLE_IS_FINITE (finitef()/__finitef() are not well supported, as far as I can tell).  Separate patch for 1.8.0 coming soon.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: validate floats, patch for 1.8.0.x

Patch for 1.8.0; untested right now (because I can't build 1.8.0 on my mac), but I'll fire up a linux vm for it.  It's pretty straightforward, though.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 246173 [details] [diff] [review]
validate floats, patch for 1.8.0.x

r=dveditz

NS_ERROR_ILLEGAL_VALUE might have been appropriate, too. But I don't feel strongly either way.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 246057 [details] [diff] [review]
fixed patch using JSDOUBLE_IS_FINITE.  patch for 1.8.1/1.9.

r=dveditz

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 246057 [details] [diff] [review]
fixed patch using JSDOUBLE_IS_FINITE.  patch for 1.8.1/1.9.

approved for 1.8 branch, a=dveditz for drivers

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 246173 [details] [diff] [review]
validate floats, patch for 1.8.0.x

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In on 1.8.0.9, 1.8.1.1

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In on trunk.

Comment 13

[Martijn Wargers (dead)]

Verified fixed on trunk and branches, by using and testing builds before the patch went in and after the patch went in.

Comment 14

[Alexander Sack]

canvasrenderingcontext does not exist on 1.7 branch. Has this code been migrated from somewhere or is this complete new code?

Comment 15

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/b12e996f5d7a