| Summary: | SIGABRT in _cairo_ft_unscaled_font_create_internal | ||
|---|---|---|---|
| Product: | cairo | Reporter: | Algunenano <raul> |
| Component: | freetype font backend | Assignee: | 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
https://github.com/matthiasclasen/cairo/commit/cb67221d030578697bdbce4e6396d5972ef4754e Not in a position to test right now, but this shoud fix it, I think. 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?!? 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 Created attachment 138460 [details] [review] Untested patch How about this? > How about this?
It works ok with that patch; it doesn't trigger the assertion any longer.
Looks right to me 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? > 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.