Last Comment Bug 372285 - Additional problems in Firefox 2.0 SVG "_cairo_pen_init"
: Additional problems in Firefox 2.0 SVG "_cairo_pen_init"
: fixed1.8.1.4
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
: Hixie (not reading bugmail)
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2007-03-01 11:28 PST by Daniel Veditz [:dveditz]
Modified: 2009-04-14 04:43 PDT (History)
5 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

add additional checking to number of pen points (1.50 KB, patch)
2007-04-19 13:46 PDT, Vladimir Vukicevic [:vlad] [:vladv]
dveditz: review+
Details | Diff | Splinter Review
add additional checking, v2 (2.11 KB, patch)
2007-04-26 13:18 PDT, Vladimir Vukicevic [:vlad] [:vladv]
dveditz: review+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2007-03-01 11:28:45 PST
Spun-off from bug 360645:

--- Comment #16  G30rgi   2007-02-25 11:49:10 PST ---

+    /* Make sure we don't overflow the size_t for malloc */
+    if (pen->num_vertices > 0xffff) {
+        return CAIRO_STATUS_NO_MEMORY;
+    }

|pen->num_vertices| seems of type |signed int|

|_cairo_pen_vertices_needed| does some operations on |doubles| and returns
*signed int*.

so negative int will pass the check and cause trouble in malloc.

is it sure that |num_vertices| is nonnegativ in all cases?

--- Comment #17 Robert C. Seacord  2007-02-28 10:48:16 PST ---

Presumably pen->num_vertices could be declared as an unsigned int as you would
not normally expect a value like this to be negative.

A safer, more localized fix may be to simply check for a lower bound, i.e.:

    if ( (pen->num_vertices <= 0) || (pen->num_vertices > 0xffff) ) {
        return CAIRO_STATUS_NO_MEMORY;

(It is usually best to eliminate the possibility of allocating zero bytes as
this behavior is implementation defined). 

--- Comment #18 G30rgi  2007-03-01 00:38:44 PST ---

another potential solution (at least for gcc) is:
(pen->num_vertices > 0xffffU)
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2007-04-19 13:46:36 PDT
Created attachment 262171 [details] [diff] [review]
add additional checking to number of pen points

Add additional checking to number of pen points before allocating.  Patch is good for 1.8.0 and 1.8.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2007-04-19 13:52:53 PDT
Er, sorry, not 1.8.0, just 1.8
Comment 3 georgi - hopefully not receiving bugspam 2007-04-19 22:54:43 PDT
hi vlad,

int num_vertices;
[1]    num_vertices = pen->num_vertices + num_points;
    if (num_vertices > 0xffff)

[1] may overflow in theory - you seem to make assumptions about |pen->num_vertices|.

if your goal is to save cpu cycles use unsigned types.

note that i don't claim [1] is a bug or is exploitable, but makes analysis harder.

Comment 4 Daniel Veditz [:dveditz] 2007-04-24 16:01:47 PDT
Comment on attachment 262171 [details] [diff] [review]
add additional checking to number of pen points

>+    if (num_points <= 0 || num_points > 0xffff)
>     num_vertices = pen->num_vertices + num_points;

I'm satisfied that pen->num_vertices is within the bounds checked by _cairo_pen_init in our current code, but Georgi's right that having a local check would have saved me a lot of work code reading to do so. It wouldn't hurt to re-check that it's not negative here--or at least assert()--in case more ways to change num_vertices are added to cairo in the future.

We can't just switch to unsigned ints because that will hellify the task of merging in updates from the cairo team.

Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2007-04-26 13:18:04 PDT
Created attachment 262927 [details] [diff] [review]
add additional checking, v2

Patch with extra check added, also pulling out magic constant into a #define.
Comment 6 Daniel Veditz [:dveditz] 2007-04-26 13:30:55 PDT
Comment on attachment 262927 [details] [diff] [review]
add additional checking, v2


approved for, a=dveditz
Comment 7 Vladimir Vukicevic [:vlad] [:vladv] 2007-04-26 13:35:29 PDT
/cvsroot/mozilla/gfx/cairo/cairo/src/cairo-pen.c,v  <--  cairo-pen.c
new revision:; previous revision:

in 1.8 branch.
Comment 8 Nochum Sossonko [:Natch] 2009-04-13 23:47:39 PDT
Resolving as FIXED, correct me if I'm wrong!
Comment 9 Jonathan Watt [:jwatt] 2009-04-14 04:43:11 PDT
I don't know if there was a reason Vlad left this open.

Note that normally you should not resolve bugs that are assigned to someone (unless you are that someone).

Note You need to log in before you can comment on or make changes to this bug.