413169 - [valgrind] cairo: uninitialized memory read during printing

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert Sayre]

Conditional jump or move depends on uninitialised value(s)
    at 0x62F12A0: _cairo_rectangle_intersect (cairo-rectangle.c:71)
    by 0x6321F54: _cairo_analysis_surface_show_glyphs (cairo-analysis-surface.c:552)
    by 0x62F4C9D: _cairo_surface_show_glyphs (cairo-surface.c:2092)
    by 0x6314E02: _cairo_meta_surface_replay_internal (cairo-meta-surface.c:815)
    by 0x631721B: _paint_page (cairo-paginated-surface.c:300)
    by 0x63174D6: _cairo_paginated_surface_show_page (cairo-paginated-surface.c:449)
    by 0x62F52B5: _moz_cairo_surface_show_page (cairo-surface.c:1708)
    by 0x62DD405: gfxPSSurface::EndPage() (gfxPSSurface.cpp:94)
    by 0x7891101: nsThebesDeviceContext::EndPage() (nsThebesDeviceContext.cpp:584)
    by 0x723B399: nsSimplePageSequenceFrame::DoPageEnd() (nsSimplePageSequence.cpp:651)
    by 0x754F290: nsPrintEngine::PrintPage(nsPrintObject*, int&) (nsPrintEngine.cpp:2344)
    by 0x755362B: nsPagePrintTimer::Notify(nsITimer*) (nsPagePrintTimer.cpp:90)

Comment 1

[Robert Sayre]

You can hit this by printing attachment 296882 [details] from bug 411369.

Comment 2

[Mats Palmgren (inactive)]

We give a cairo_rectangle_int_t* to cairo_scaled_font_glyph_device_extents()
that expects a cairo_rectangle_int16_t*:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/cairo/cairo/src/cairo-analysis-surface.c&rev=1.13&root=/cvsroot&mark=515,545,548,552#505
so 'glyph_extents' will have some uninitialized bytes, which 
cairo_rectangle_intersect() uses, which valgrind complains about.

gcc actually warns about the problem:
cairo-analysis-surface.c: In function '_cairo_analysis_surface_show_glyphs':
cairo-analysis-surface.c:548: warning: passing argument 4 of '_cairo_scaled_font_glyph_device_extents' from incompatible pointer type

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I changed cairo_scaled_font_glyph_device_extents() to take a
cairo_rectangle_int_t* instead.

Comment 4

[Reed Loden [:reed]]

(In reply to comment #2)
> gcc actually warns about the problem:
> cairo-analysis-surface.c: In function '_cairo_analysis_surface_show_glyphs':
> cairo-analysis-surface.c:548: warning: passing argument 4 of
> '_cairo_scaled_font_glyph_device_extents' from incompatible pointer type

Why again do we not use warnings-as-errors in more places? Warnings are not usually good, yet we seem to ignore them throughout the tree. :(

Comment 5

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

Comment on attachment 298117 [details] [diff] [review]
Patch rev. 1


Ah ha, good catch -- I guess I need to audit usage of rectangle_int16_t again, since either I missed some or some snuck in after my big change.

I'll get this into upstream cairo.

Comment 6

[Mats Palmgren (inactive)]

mozilla/gfx/cairo/cairo/src/cairo-scaled-font.c 	1.27
mozilla/gfx/cairo/cairo/src/cairo-surface-fallback.c 	1.23
mozilla/gfx/cairo/cairo/src/cairo-xcb-surface.c 	1.24
mozilla/gfx/cairo/cairo/src/cairo-xlib-surface.c 	1.32
mozilla/gfx/cairo/cairo/src/cairoint.h 	1.52
mozilla/gfx/cairo/cairo/src/cairo-analysis-surface.c 	1.14 

-> FIXED

Comment 7

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

committed upstream.