Bug 105746

Summary: SIGABRT in _cairo_ft_unscaled_font_create_internal
Product: cairo Reporter: Algunenano <raul>
Component: freetype font backendAssignee: Matthias Clasen <mclasen>
Status: RESOLVED FIXED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: normal    
Priority: medium CC: freedesktop, kalevlember, tom
Version: unspecified   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: Untested patch

Description Algunenano 2018-03-26 11:18:11 UTC
Testing with the current master (`1ed124ace  Fix a 'memory leak' in the image compositor`) I'm getting a SIGABRT when running Mapnik's visual test.

Here is the callstack:
```
==15411== Process terminating with default action of signal 6 (SIGABRT): dumping core
==15411==    at 0x9567860: raise (in /usr/lib/libc-2.26.so)
==15411==    by 0x9568EC8: abort (in /usr/lib/libc-2.26.so)
==15411==    by 0x95600BB: __assert_fail_base (in /usr/lib/libc-2.26.so)
==15411==    by 0x9560132: __assert_fail (in /usr/lib/libc-2.26.so)
==15411==    by 0x61F0616: _cairo_ft_unscaled_font_create_internal (cairo-ft-font.c:576)
==15411==    by 0x61F0616: _cairo_ft_unscaled_font_create_from_face (cairo-ft-font.c:643)
==15411==    by 0x61F0616: cairo_ft_font_face_create_for_ft_face (cairo-ft-font.c:3707)
==15411==    by 0x545BA7C: mapnik::cairo_face::cairo_face(std::shared_ptr<mapnik::font_library> const&, std::shared_ptr<mapnik::font_face> const&) (cairo_context.cpp:39)
==15411==    by 0x545CDD2: construct<mapnik::cairo_face, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (new_allocator.h:136)
==15411==    by 0x545CDD2: construct<mapnik::cairo_face, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (alloc_traits.h:475)
==15411==    by 0x545CDD2: _Sp_counted_ptr_inplace<std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr_base.h:526)
==15411==    by 0x545CDD2: __shared_count<mapnik::cairo_face, std::allocator<mapnik::cairo_face>, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr_base.h:637)
==15411==    by 0x545CDD2: __shared_ptr<std::allocator<mapnik::cairo_face>, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr_base.h:1294)
==15411==    by 0x545CDD2: shared_ptr<std::allocator<mapnik::cairo_face>, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr.h:344)
==15411==    by 0x545CDD2: allocate_shared<mapnik::cairo_face, std::allocator<mapnik::cairo_face>, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr.h:690)
==15411==    by 0x545CDD2: make_shared<mapnik::cairo_face, std::shared_ptr<mapnik::font_library> &, std::shared_ptr<mapnik::font_face> &> (shared_ptr.h:706)
==15411==    by 0x545CDD2: mapnik::cairo_face_manager::get_face(std::shared_ptr<mapnik::font_face>) (cairo_context.cpp:526)
==15411==    by 0x545CAD0: mapnik::cairo_context::set_font_face(mapnik::cairo_face_manager&, std::shared_ptr<mapnik::font_face>) (cairo_context.cpp:374)
==15411==    by 0x545D335: mapnik::cairo_context::add_text(mapnik::glyph_positions const&, mapnik::cairo_face_manager&, mapnik::composite_mode_e, mapnik::composite_mode_e, double) (cairo_context.cpp:469)
==15411==    by 0x5461875: mapnik::cairo_renderer<std::shared_ptr<_cairo> >::process(mapnik::text_symbolizer const&, mapnik::feature_impl&, mapnik::proj_transform const&) (process_text_symbolizer.cpp:99)
==15411==    by 0x4F7D15E: apply_const (variant.hpp:317)
==15411==    by 0x4F7D15E: visit<mapnik::symbolizer_dispatch<mapnik::cairo_renderer<mapnik::cairo_ptr> >, mapbox::util::variant<mapnik::point_symbolizer, mapnik::line_symbolizer, mapnik::line_pattern_symbolizer, mapnik::polygon_symbolizer, mapnik::polygon_pattern_symbolizer, mapnik::raster_symbolizer, mapnik::shield_symbolizer, mapnik::text_symbolizer, mapnik::building_symbolizer, mapnik::markers_symbolizer, mapnik::group_symbolizer, mapnik::debug_symbolizer, mapnik::dot_symbolizer>, void> (variant.hpp:864)
==15411==    by 0x4F7D15E: apply_visitor<mapnik::symbolizer_dispatch<mapnik::cairo_renderer<mapnik::cairo_ptr> >, mapbox::util::variant<mapnik::point_symbolizer, mapnik::line_symbolizer, mapnik::line_pattern_symbolizer, mapnik::polygon_symbolizer, mapnik::polygon_pattern_symbolizer, mapnik::raster_symbolizer, mapnik::shield_symbolizer, mapnik::text_symbolizer, mapnik::building_symbolizer, mapnik::markers_symbolizer, mapnik::group_symbolizer, mapnik::debug_symbolizer, mapnik::dot_symbolizer> > (variant.hpp:42)
==15411==    by 0x4F7D15E: mapnik::feature_style_processor<mapnik::cairo_renderer<std::shared_ptr<_cairo> > >::render_style(mapnik::cairo_renderer<std::shared_ptr<_cairo> >&, mapnik::feature_type_style const*, mapnik::rule_cache const&, std::shared_ptr<mapnik::Featureset>, mapnik::proj_transform const&) (feature_style_processor_impl.hpp:621)
==15411==    by 0x4F7B142: mapnik::feature_style_processor<mapnik::cairo_renderer<std::shared_ptr<_cairo> > >::render_material(mapnik::layer_rendering_material const&, mapnik::cairo_renderer<std::shared_ptr<_cairo> >&) (feature_style_processor_impl.hpp:573)
```


This was working correctly with `1.15.10` and I've bisected the changes and found that the issue was introduced in `42f07ef90 Always save the origin face index`. Reverting that commit on top of the current master/HEAD solves the issue.
Comment 1 Matthias Clasen 2018-03-29 16:31:42 UTC
https://github.com/matthiasclasen/cairo/commit/cb67221d030578697bdbce4e6396d5972ef4754e

Not in a position to test right now, but this shoud fix it, I think.
Comment 2 Uli Schlachter 2018-03-29 17:35:52 UTC
Thanks for the patch. However, it looks like it tries to fix a null pointer dereference (SIGSEGV). However, this bug report is about an assertion failure (SIGABRT).
The failing assertion is

  assert (unscaled->base.hash_entry.hash == key.base.hash_entry.hash);

What the code does is: It first checks the cache (a hash table) for an entry. If no entry is found, it creates a new unscaled font. The assertion here is that the hash code of this new unscaled font is the same as what was used for the negative lookup before.

The hash code is calculated based on the "id" argument to _cairo_ft_unscaled_font_init(). However, your patch uses face->face_index instead for calculating the hash code. Thus, if face->face_index != id, there is a non-zero chance of a different hash code, which is not allowed.

Since your original patch replaced "0" with "face->face_index", why not replace the "0" with "id" instead?

(Note that I still have no clue about this stuff, but I just looked at the code that surrounded the assertion and then noticed that I ended up looking at exactly the code that was changed in 42f07ef90).

By the way: Thank you Algunenano for the good bug report. The bisection was very helpful.

Oh and: The commit message for 42f07ef90 says that ft_face->face_index changes "underneath us" as font variations are applied. That sounds really, really troublesome, since this means that the key of the hash table is changed without updating the hash table?!?
Comment 3 Matthias Clasen 2018-03-29 18:15:12 UTC
Oh, I didn't read the stacktrace carefully enough. At this point, I think we need Behdad to comment. He's the master of font caching
Comment 4 Behdad Esfahbod 2018-03-31 10:30:27 UTC
Created attachment 138460 [details] [review]
Untested patch

How about this?
Comment 5 Algunenano 2018-03-31 12:55:20 UTC
> How about this?

It works ok with that patch; it doesn't trigger the assertion any longer.
Comment 6 Matthias Clasen 2018-04-01 23:14:43 UTC
Looks right to me
Comment 7 Algunenano 2018-04-16 14:00:21 UTC
Hi there, 1.15.12 was published but the `assert` is still failing. Applying Behdad's patch solves it for me. Is there anything I can do to move this forward so it gets fixed?
Comment 8 Uli Schlachter 2018-04-21 07:46:32 UTC
> Is there anything I can do to move this forward so it gets fixed?

Writing a comment on this bug and asking for just that seems to be a good idea. :-)

Pushed the patch from comment 4.

commit 7554822dd0b52d33ec7898e81b59e97164b00142
Author: Uli Schlachter <psychon@znc.in>
Date:   Sat Apr 21 09:37:06 2018 +0200

    Fix assertion failure in the freetype backend
    
    Fonts are kept in a hash table, so when creating a new font, the code
    first checks the hash table for an already-existing entry and only then
    is a new instance really created. There is an assert that checks that
    the key used for the hash table lookup is the same as the instance that
    is created later has, because otherwise the hash table was checked
    incorrectly.
    
    This assert failed in some conditions.
    
    Fix this by fixing some places that initialised ft hash keys in a wrong
    way.
    
    Patch by Behdad Esfahbod and submitted via bugzilla.
    
    Source: https://bugs.freedesktop.org/show_bug.cgi?id=105746#c4
    Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=105746
    Signed-off-by: Uli Schlachter <psychon@znc.in>

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.