Closed
Bug 372285
Opened 18 years ago
Closed 16 years ago
Additional problems in Firefox 2.0 SVG "_cairo_pen_init"
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dveditz, Assigned: vlad)
Details
(Keywords: fixed1.8.1.4, Whiteboard: [sg:investigate])
Attachments
(2 files)
1.50 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
dveditz
:
review+
dveditz
:
approval1.8.1.4+
|
Details | Diff | Splinter Review |
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)
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.3+
Assignee | ||
Comment 1•18 years ago
|
||
Add additional checking to number of pen points before allocating. Patch is good for 1.8.0 and 1.8.
Attachment #262171 -
Flags: review?(dveditz)
Assignee | ||
Comment 2•18 years ago
|
||
Er, sorry, not 1.8.0, just 1.8
Comment 3•18 years ago
|
||
hi vlad,
int num_vertices;
...
[1] num_vertices = pen->num_vertices + num_points;
if (num_vertices > 0xffff)
return CAIRO_STATUS_NO_MEMORY;
[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.
Reporter | ||
Updated•18 years ago
|
Whiteboard: needs r=dveditz
Reporter | ||
Comment 4•18 years ago
|
||
Comment on attachment 262171 [details] [diff] [review]
add additional checking to number of pen points
>+ if (num_points <= 0 || num_points > 0xffff)
>+ return CAIRO_STATUS_NO_MEMORY;
>+
> 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.
r=dveditz
Attachment #262171 -
Flags: review?(dveditz) → review+
Assignee | ||
Comment 5•18 years ago
|
||
Patch with extra check added, also pulling out magic constant into a #define.
Attachment #262927 -
Flags: review?(dveditz)
Attachment #262927 -
Flags: approval1.8.1.4?
Reporter | ||
Comment 6•18 years ago
|
||
Comment on attachment 262927 [details] [diff] [review]
add additional checking, v2
thanks
r=dveditz
approved for 1.8.1.4, a=dveditz
Attachment #262927 -
Flags: review?(dveditz)
Attachment #262927 -
Flags: review+
Attachment #262927 -
Flags: approval1.8.1.4?
Attachment #262927 -
Flags: approval1.8.1.4+
Assignee | ||
Comment 7•18 years ago
|
||
/cvsroot/mozilla/gfx/cairo/cairo/src/cairo-pen.c,v <-- cairo-pen.c
new revision: 1.1.4.4; previous revision: 1.1.4.3
in 1.8 branch.
Keywords: fixed1.8.1.4
Whiteboard: needs r=dveditz
Reporter | ||
Updated•18 years ago
|
Whiteboard: [sg:investigate]
Reporter | ||
Updated•18 years ago
|
Group: security
Comment 8•16 years ago
|
||
Resolving as FIXED, correct me if I'm wrong!
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
![]() |
||
Comment 9•16 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•