376498 - Stroking bitmap fonts stops rendering

Status: RESOLVED WORKSFORME

(Core :: SVG, defect, P3)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Opening the page in the URL causes a crash in cario-path-fixed.c at line 139 in function _cairo_path_fixed_fini

Unhandled exception at 0x0149248e (thebes.dll) in firefox.exe: 0xC0000005: Access violation reading location 0xdddddddd.

Comment 1

[Robert Longson [:longsonr]]

_cairo_scaled_glyph_lookup calls 
  _cairo_win32_scaled_font_init_glyph_path which calls
     GetGlyphOutlineW which fails (prints_cairo_win32_scaled_font_init_glyph_metrics:GetGlyphOutlineW: Cannot complete this function).

The code jumps to CLEANUP_FONT in _cairo_win32_scaled_font_init_glyph_path
CLEANUP_FONT calls _cairo_scaled_glyph_set_path which sets a path into scaled_glyph
The memory for this path is then freed by calling _cairo_path_fixed_destroy.
Scaled_glyph->path now points to freed memory

_cairo_scaled_glyph_lookup calls _cairo_scaled_flyph_fini because _cairo_win32_scaled_font_init_glyph_path failed
and this calls _cairo_path_fixed_destroy so freeing the memory again and we crash.

Comment 2

[tor]

Attachment: simpler testcase

Doesn't crash when loaded, but prints the same error and puts cairo into an error state.  The font-family="courier" appears to be the key for this behavior.  Bitmap font that we're trying to turn into a path, maybe?

Comment 3

[Jonathan Watt [:jwatt]]

As of the update to cairo 1.4.2 this hangs on Windows XP instead of crashing.

Comment 4

[Jesse Ruderman]

Crashes dereferencing 0xdddddddd are often security holes.  Is that crash present on branch or only on certain trunk builds?

Comment 5

[Robert Longson [:longsonr]]

The change that I think broke things went in to the cairo git trunk on Oct 31 last year. The URL does crash my windows debug trunk build and it has cairo 1.4.2.

FX 2 is OK as its cairo is too old to exhibit this problem.

Comment 6

[Carl Worth]

(In reply to comment #5)
> The change that I think broke things went in to the cairo git trunk on Oct 31
> last year. The URL does crash my windows debug trunk build and it has cairo
> 1.4.2.

Could you provide some details on where that Oct 31 date comes from? I see cairo commits that landed with a commit date of 2006-10-29 and 2007-11-02, but nothing in between, (I did have to take my kids out in quest of candy after all...).

-Carl

Comment 7

[Jonathan Watt [:jwatt]]

I have a range of tinderbox builds here, and they crash all the way back to 2005-12-25-05-trunk. Not being debug builds, I don't know if they have the same stack. (And when did cairo get turned on by default again?)

Comment 8

[Carl Worth]

Attachment: 0001-Move-misplaced-CLEANUP_FONT-label-to-avoid-crash.txt

This patch attempts to avoid the double-free crash, (thanks for the careful description above).

It doesn't attempt to address the error state apparently caused by cairo's win32 backend trying in vain to extract a path from a bitmap font.

-Carl

Comment 9

[Carl Worth]

(In reply to comment #8)
> This patch attempts to avoid the double-free crash, (thanks for the careful
> description above).

By the way, I cannot test this patch, but if someone can test it for me and verify that it prevents the crash, then I will commit it to upstream cairo.

> It doesn't attempt to address the error state apparently caused by cairo's
> win32 backend trying in vain to extract a path from a bitmap font.

By the other way, higher layers in cairo already know how to handle the case where a bitmap font cannot provide a path, (cairo instead obtains the bitmap image and traces a path as a set of rectangles around each pixel).

So all that really needs to happen here is that _cairo_win32_scaled_font_glyph_init should be fixed to return CAIRO_INT_STATUS_UNSUPPORTED in the case where the info parameter has the CAIRO_SCALED_GLYPH_INFO_PATH bit set and the font is a bitmap font.

I don't know anything about win32 fonts, GDI functions or their error codes, but presumably the return value of the currently failing GetOutlineW could be keyed off of to detect this case, (or perhaps something could be done earlier).

Regardless, cairo-win32 should definitely not be printing anything to the console just because it gets asked to do something unsupported, (that will simply be worked around by the calling code).

-Carl

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #6)
> Could you provide some details on where that Oct 31 date comes from? I see
> cairo commits that landed with a commit date of 2006-10-29 and 2007-11-02, but
> nothing in between, (I did have to take my kids out in quest of candy after
> all...).
> 
> -Carl
> 

Sorry, I meant Oct 31 2005...

http://gitweb.freedesktop.org/?p=cairo.git;a=commitdiff;h=3cae05c4c503ce71c4967bd3f748cdfa3bb76ebc;hp=1a25220634013c4ef475f92110ede366e0847572

Comment 11

[Carl Worth]

(In reply to comment #10)

> Sorry, I meant Oct 31 2005...

Ah, that makes more sense! :-)

> http://gitweb.freedesktop.org/?p=cairo.git;a=commitdiff;h=3cae05c4c503ce71c4967bd3f748cdfa3bb76ebc;hp=1a25220634013c4ef475f92110ede366e0847572

Thanks. For future reference, this is _much_ more useful than a date, (and even just saying commit 3cae05c4c5 would have been good enough for me, but some people like the gitweb URLs).

Anyway, yes, that does confirm my original patch. The new code added in this commit should have been added before the CLEANUP_FONT label, and not after it.

But we'll still need someone to properly fix things to return CAIRO_INT_STATUS_UNSUPPORTED when a path is requested for a bitmap glyph, (I just sent a request for that off to the cairo mailing list).

-Carl

Comment 12

[Robert Longson [:longsonr]]

(In reply to comment #11)

Your patch is necessary, it fixes the double memory free, but as you suspected insufficient.

The call to _cairo_win32_print_gdi_error makes _cairo_win32_scaled_font_init_glyph_path return CAIRO_STATUS_NO_MEMORY. This eventually makes its way into cr->status and cairo goes into an error state from which it never recovers. The browser crashes shortly afterwards.

Making _cairo_win32_scaled_font_init_glyph_path return CAIRO_INT_STATUS_UNSUPPORTED instead makes _cairo_scaled_font_glyph_path call _cairo_scaled_glyph_lookup with info of CAIRO_SCALED_GLYPH_INFO_SURFACE. This then asserts in _cairo_win32_scaled_font_glyph_init.

Comment 13

[Carl Worth]

(In reply to comment #12)
> Your patch is necessary, it fixes the double memory free,

Thanks. With that, I've pushed it out now to upstream cairo:

http://gitweb.freedesktop.org/?p=cairo;a=commit;h=106f8590457a7ebb5335d67f16277e8d5a6b04a8

> but as you suspected insufficient.

As you've seen already, we're discussing this on the cairo mailing list. We really do need some help from somebody with some win32 clue though.

-Carl

Comment 14

[chris hofmann]

tor/pav, can you provide that win32 help?

Comment 15

[Damon Sicore (:damons)]

Pav, can you please follow up on this and make sure any upstream issues are resolved?

Comment 16

[Stuart Parmenter]

tor? do you know what is going on here?  I don't know the path stuff at all.

Comment 17

[Damon Sicore (:damons)]

Setting to B1 per Stuart.

Comment 18

[Damon Sicore (:damons)]

Tor, Stuart, any news on this?

Comment 19

[Robert Longson [:longsonr]]

This no longer either crashes or hangs for me but it does not display the web page properly either. Cairo still goes into an error state - see comment 12 for why. So I don't think this bug is security sensitive any more.

Firefox 2 displays the page reasonably well so we have regressed in our ability to display this page.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

I no longer see a crash or hang using firefox3.0b2pre Gecko/2007113005 on WinXP and the page seems to look identical to Firefox 2.

Comment 21

[Robert Longson [:longsonr]]

The crash and hang were fixed some time ago by separate check-ins. I am trying now with Gecko/2007120910 and the page doesn't display. The page that was there previously displays.

Here are links to the cairo discussions referred to in comment 13

http://lists.cairographics.org/archives/cairo/2007-April/010273.html
http://lists.cairographics.org/archives/cairo/2007-April/010288.html
http://lists.cairographics.org/archives/cairo/2007-April/010291.html
http://lists.cairographics.org/archives/cairo/2007-April/010295.html
http://lists.cairographics.org/archives/cairo/2007-April/010312.html
http://lists.cairographics.org/archives/cairo/2007-April/010317.html
http://lists.cairographics.org/archives/cairo/2007-April/010324.html

That's as far as the cairo discussion went and _cairo_win32_scaled_font_glyph_init still doesn't support CAIRO_SCALED_GLYPH_INFO_SURFACE

Comment 22

[Daniel Veditz [:dveditz]]

This now seems to be WFM by several people who just tested it in our security triage mtg. Can this be closed now or is there still some issue?

Comment 23

[Daniel Veditz [:dveditz]]

On windows the "simpler testcase" is blank instead of showing a blue word "bar". But this behavior is consistent with Firefox 2.0.0.11 so it can't be _this_ bug which is a regression.

Comment 24

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

(In reply to comment #23)
> On windows the "simpler testcase" is blank instead of showing a blue word
> "bar". But this behavior is consistent with Firefox 2.0.0.11 so it can't be
> _this_ bug which is a regression.

Filed bug 411778 on that.

Comment 25

[Robert Longson [:longsonr]]

(In reply to comment #22)
> This now seems to be WFM by several people who just tested it in our security
> triage mtg. Can this be closed now or is there still some issue?
> 

When you click on http://www.intertwingly.net/blog/archives/2006/11/ you should see a calendar. I've a build which is a few minutes old and although it neither hangs nor crashes it does not display the calendar either; it looks like you have not gone anywhere as the bugzilla page is still displayed (same with the simpler testcase). Parts of the calendar will display if you show and hide bits of the screen. Note that it is only cairo on Windows which has this issue, other platforms are fine.

Comment 26

[Robert Longson [:longsonr]]

Now works for me too.