371638 - potential cairo-hull int overflow

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo-hull.c&rev=1.3&mark=65#65

65                  hull = malloc (num_vertices * sizeof (cairo_hull_t));
is the same construct as
Bug 360645 – Firefox 2.0 SVG "_cairo_pen_init"

but can't hit it yet.

Comment 1

[Daniel Veditz [:dveditz]]

Are you sure we can't call this? I stopped following when it looked like three separate places could trigger this.

3 callers ->
_cairo_path_fixed_stroke_to_traps
_cairo_path_fixed_interpret(... _cairo_stroker_curve_to ...)
_cairo_pen_add_points
_cairo_hull_compute
_cairo_hull_init

Do these cairo fixes make it into the upstream? Or do we have to worry about them recurring next time we merge new stuff in?

Comment 2

[georgi - hopefully not receiving bugspam]

(In reply to comment #1)
> Are you sure we can't call this? I stopped following when it looked like three
> separate places could trigger this.
> 
> 3 callers ->
> _cairo_path_fixed_stroke_to_traps
> _cairo_path_fixed_interpret(... _cairo_stroker_curve_to ...)
> _cairo_pen_add_points
> _cairo_hull_compute
> _cairo_hull_init
> 
> Do these cairo fixes make it into the upstream? Or do we have to worry about
> them recurring next time we merge new stuff in?
> 

in case the comment was to me:
not sure that can't hit it, just pointed out couldn't hit it yesterday evening.
and even if can't be hitted at the moment, it is may be hittable by a future caller.

Comment 3

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

I have a patch that I'm waiting to get into cairo proper that fixes this and all similar issues; it basically adds a number of helper functions for malloc(N*k) and (N*Z*k) etc.

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #3)
> I have a patch that I'm waiting to get into cairo proper that fixes this and
> all similar issues; it basically adds a number of helper functions for
> malloc(N*k) and (N*Z*k) etc.
> 

isn't malloc(N*k) calloc(N,k) (calloc seems to check for overflow on linux)?

DESCRIPTION
       calloc()  allocates memory for an array of nmemb elements of size bytes
       each and returns a pointer to the allocated memory.  The memory is  set
       to zero.

Comment 5

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

It is, except that it also zeros the memory, which is unnecessary in these cases; there are other bits that wouldn't be covered by calloc (a*b*k and a*k+s) so there'll just be a unified way to do those allocations.  Should get the patch into cairo and into moz before a6.

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> It is, except that it also zeros the memory, which is unnecessary in these

yes it zeros, agreed

Comment 7

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

This should be fixed now -- all cairo allocations are overflow-checked.

Comment 8

[Daniel Veditz [:dveditz]]

Do we need to worry about this on the 1.8 branch?

Comment 9

[Samuel Sidler (old account; do not CC)]

Vlad, can we get an answer to comment 8?

Comment 10

[Daniel Veditz [:dveditz]]

no one wants to backport these fixes without knowing this code can be exercised with FF2's limited use of cairo. Not going to block on it for 1.8.1.15 but needs more investigation.