Bug 191595 - [FreeType] Problem under WebCore::FontPlatformData::FontPlatformData
Summary: [FreeType] Problem under WebCore::FontPlatformData::FontPlatformData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-13 13:17 PST by Michael Catanzaro
Modified: 2018-11-21 08:52 PST (History)
6 users (show)

See Also:


Attachments
Full backtrace (179.40 KB, text/plain)
2018-11-13 13:17 PST, Michael Catanzaro
no flags Details
Patch (3.08 KB, patch)
2018-11-20 08:19 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2018-11-20 18:45 PST, Michael Catanzaro
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.80 MB, application/zip)
2018-11-20 20:41 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-11-13 13:17:02 PST
Created attachment 354692 [details]
Full backtrace

Reproducer: https://thepointsguy.com/2017/09/experience-only-on-virgin-america/

Full backtrace attached. Truncated backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fee8eade515 in __GI_abort () at abort.c:79
#2  0x00007fee8eb354c8 in __libc_message (action=action@entry=do_abort, 
    fmt=fmt@entry=0x7fee8ec3e1fb "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007fee8eb3b8da in malloc_printerr (str=str@entry=0x7fee8ec3c402 "free(): invalid pointer")
    at malloc.c:5350
#4  0x00007fee8eb3d06c in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>)
    at malloc.c:4157
#5  0x00007fee8fedd993 in cairo_ft_apply_variations (face=face@entry=0x7fed3aba7100, 
    scaled_font=<optimized out>, scaled_font=<optimized out>) at ../../src/cairo-ft-font.c:2396
#6  0x00007fee8fee0a7f in cairo_ft_scaled_font_lock_face (
    abstract_font=abstract_font@entry=0x55c6020ea1b0) at ../../src/cairo-ft-font.c:3859
#7  0x00007fee95085334 in WebCore::CairoFtFaceLocker::CairoFtFaceLocker (scaledFont=0x55c6020ea1b0, 
    this=<synthetic pointer>) at ../Source/WebCore/platform/graphics/cairo/CairoUtilities.h:55
#8  WebCore::FontPlatformData::FontPlatformData (this=0x7ffc53bd7d30, fontFace=<optimized out>, 
    description=..., bold=<optimized out>, italic=<optimized out>)
    at ../Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:168
#9  0x00007fee95084873 in WebCore::FontCustomPlatformData::fontPlatformData (this=<optimized out>, 
    description=..., bold=<optimized out>, italic=<optimized out>)
    at ../Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:61
#10 0x00007fee949767f4 in WebCore::CachedFont::platformDataFromCustomData (fontCustomPlatformData=..., 
    fontDescription=..., bold=<optimized out>, italic=<optimized out>, fontFaceFeatures=..., 
    fontFaceVariantSettings=..., fontFaceCapabilities=...)
    at ../Source/WebCore/loader/cache/CachedFont.cpp:150
Comment 1 Michael Catanzaro 2018-11-13 13:17:23 PST
(This is with 2.22.3)
Comment 2 Carlos Garcia Campos 2018-11-19 02:24:32 PST
I can't reproduce this.
Comment 3 Carlos Garcia Campos 2018-11-19 03:40:38 PST
Looking at the bt, I think the problem is that cairo is trying to free a FT_MM_Var with free when using freetype 2.9. I can't reproduce it because i have freetype 2.8.1.
Comment 4 Carlos Garcia Campos 2018-11-19 03:41:22 PST
See https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5
Comment 5 Michael Catanzaro 2018-11-19 08:08:17 PST
Thanks
Comment 6 Michael Catanzaro 2018-11-20 07:35:29 PST
Reopening
Comment 7 Michael Catanzaro 2018-11-20 07:41:56 PST
Closing again. :)
Comment 8 Michael Catanzaro 2018-11-20 07:42:25 PST
Actually let's use this bug to workaround the issue based on cairo version.
Comment 9 Michael Catanzaro 2018-11-20 08:19:28 PST
Created attachment 355346 [details]
Patch
Comment 10 Adrian Perez 2018-11-20 12:24:47 PST
Comment on attachment 355346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355346&action=review

There are some changes needed in this patch, please take a look.

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:71
> +    if (version >= CAIRO_VERSION_ENCODE(1, 15, 0) && version < CAIRO_VERSION_ENCODE(1, 16, 1)) {

If I understand correctly, we want to *avoid* using fastMalloc() and friends
for Cairo 1.16.0, but here the code is *enabling* their usage for Cairo
versions between [1.15.0, 1.16.1) — which goes against the comment above.
I think here the condition should be reversed:

    if (version < CAIRO_VERSION_ENCODE(1, 16, 0) && version >= CAIRO_VERSION_ENCODE(1, 16, 1))

This way the custom memory allocator is set for every version except the ones
in the range [1.16.0, 1.16.1).

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:91
>      }

When “FT_New_Library” is not being used, FreeType is not being initialized
at all! You will want to initialize the library using “FT_Init_FreeType()”
in an else branch. The following is missing:

   else {
       if (FT_Init_FreeType(&library))
           return false;
   }

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:93
>      FT_Add_Default_Modules(library);

You will want to move this call to “FT_Add_Default_Modules()” above, right
after using “FT_New_Library()”. It is not needed to call this when using
“FT_Init_FreeType()” because it's called implicitly in that case.
Comment 11 Michael Catanzaro 2018-11-20 15:30:26 PST
Comment on attachment 355346 [details]
Patch

Good catches, it's a disaster. Let me try again.
Comment 12 Michael Catanzaro 2018-11-20 18:45:19 PST
Created attachment 355376 [details]
Patch
Comment 13 Michael Catanzaro 2018-11-20 18:47:36 PST
Here's a fixed patch for unofficial review. We might as well just commit this to stable and not pollute trunk with it.
Comment 14 Build Bot 2018-11-20 20:41:06 PST
Comment on attachment 355376 [details]
Patch

Attachment 355376 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10094493

New failing tests:
webanimations/leak-document-with-web-animation.html
Comment 15 Build Bot 2018-11-20 20:41:17 PST
Created attachment 355380 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 16 Carlos Garcia Campos 2018-11-21 01:14:26 PST
Comment on attachment 355376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355376&action=review

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:71
> +    // Workaround crash in cairo 1.16.0. See https://webkit.org/b/191595.
> +    int version = cairo_version();
> +    if (version >= CAIRO_VERSION_ENCODE(1, 15, 0) && version < CAIRO_VERSION_ENCODE(1, 16, 1)) {

This is only broken if freetype version is >= 2.9
Comment 17 Michael Catanzaro 2018-11-21 04:31:36 PST
Are you sure? I can't reproduce the crash anymore either: that doesn't mean it's not a security problem. FT_Get_MM_Var was added in 2004 and I can see it's been using FT_ALLOC (in TT_Get_MM_Var) since then, so my guess is it has never been tested with custom allocators and the first cairo version to use it (1.16) is the first bad version.
Comment 18 Carlos Garcia Campos 2018-11-21 06:12:45 PST
Yes, I guess previous versions of freetype used malloc directly, the docs said you had to free it with free (not with FC functions). It doesn't crash for me with current cairo.
Comment 19 Michael Catanzaro 2018-11-21 08:32:52 PST
(In reply to Carlos Garcia Campos from comment #18)
> Yes, I guess previous versions of freetype used malloc directly, the docs
> said you had to free it with free (not with FC functions).

I think the docs were wrong and there was no safe way to use it before.

> It doesn't crash for me with current cairo.

Me either, that's just luck I think.
Comment 20 Michael Catanzaro 2018-11-21 08:52:39 PST
Committed workaround to 2.22 branch only. This should be fixed in cairo long before 2.24 and we don't have to argue about whether to check the freetype version or not if it's only on 2.22. :)