Huge multi-threading violations in freetype

[Brad Figg]

SRU Justification
[Impact]
Users of freetype in multithreaded environments can experiences crashes.

[Test Case]
https://github.com/behdad/ftthread

[Regression Potential]
Fix is in upstream, it is in Vivid.

--

Fresh dist-upgrade; brought up the dash, typed "term"; single clicked on the terminal app and _bang_.

ProblemType: Crash
DistroRelease: Ubuntu 13.10
Package: unity 7.0.2+13.10.20130705.1-0ubuntu1
ProcVersionSignature: Ubuntu 3.10.0-2.10-generic 3.10.0
Uname: Linux 3.10.0-2-generic x86_64
ApportVersion: 2.10.2-0ubuntu4
Architecture: amd64
Date: Tue Jul 9 17:47:10 2013
EcryptfsInUse: Yes
ExecutablePath: /usr/bin/compiz
InstallationDate: Installed on 2013-06-06 (33 days ago)
InstallationMedia: Ubuntu 13.04 "Raring Ringtail" - Release amd64 (20130424)
MarkForUpload: True
ProcCmdline: compiz
ProcEnviron:
 LANGUAGE=en_US
 PATH=(custom, user)
 XDG_RUNTIME_DIR=<set>
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SegvAnalysis:
 Segfault happened at: 0x7ff3b4ed7a10: mov %rdx,0x10(%rax)
 PC (0x7ff3b4ed7a10) ok
 source "%rdx" ok
 destination "0x10(%rax)" (0x00000010) not located in a known VMA region (needed writable region)!
 Stack memory exhausted (SP below stack segment)
SegvReason: writing NULL VMA
Signal: 11
SourcePackage: unity
StacktraceTop:
 ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
 ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
 ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
 ?? () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
 FT_Load_Glyph () from /usr/lib/x86_64-linux-gnu/libfreetype.so.6
Title: compiz crashed with SIGSEGV in FT_Load_Glyph()
UpgradeStatus: No upgrade log present (probably fresh install)
UserGroups: adm cdrom dip lpadmin plugdev sambashare sudo

[Apport retracing service (apport)]

StacktraceTop:
 af_latin_hints_compute_edges (dim=<optimized out>, hints=<optimized out>) at /build/buildd/freetype-2.4.12/freetype-2.4.12/src/autofit/aflatin.c:1271
 af_latin_hints_detect_features (hints=<optimized out>, dim=<optimized out>) at /build/buildd/freetype-2.4.12/freetype-2.4.12/src/autofit/aflatin.c:1411
 af_latin_hints_apply (hints=0x1953d58, outline=0x1953ee0, metrics=0x7ff37002c950) at /build/buildd/freetype-2.4.12/freetype-2.4.12/src/autofit/aflatin.c:2381
 af_loader_load_g (loader=loader@entry=0x1953d40, scaler=scaler@entry=0x7ff383feb790, glyph_index=glyph_index@entry=33, load_flags=load_flags@entry=68097, depth=depth@entry=0) at /build/buildd/freetype-2.4.12/freetype-2.4.12/src/autofit/afloader.c:184
 af_loader_load_glyph (load_flags=68097, gindex=<optimized out>, face=<optimized out>, module=<optimized out>) at /build/buildd/freetype-2.4.12/freetype-2.4.12/src/autofit/afloader.c:553

[Marek Kašík (mkasik)]

Created attachment 85352
Make usage of freetype thread safe

Current freetype backend is not thread safe. It can cause segmentation fault when threads are not properly handled by applications which use cairo (see https://bugzilla.redhat.com/show_bug.cgi?id=678397 and its duplicates).

Attached patch tries to fix this problem by creating FT_Library object for each thread. This is recommended solution by freetype (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/include/freetype/freetype.h#n1684).

It adds a hash table to which FT_Library is added if a new thread needs one. Thread IDs are used as keys for this table.

Marek

[Freedesktop (freedesktop)]

Unfortunately that FreeType "recommendation" is red herring. First, people are free to pass around FT_Face among threads, and second, we will be leaking stuff this way, unless we add proper TLS support.

At any rate. I suggest we use cairo for all glyph rasterizer instead.

[Freedesktop (freedesktop)]

Chris, any chance you have some time to think about this?

[Sebastien Bacher (seb128)]

e.u.c report on https://errors.ubuntu.com/problem/62a5e02a5ec4ed62cd98c21d2ff80a5cdf6aec47

[Alexey Borzenkov (snaury)]

I looked at my coredump on trusty (it crashed for me twice already!) with gdb and I see evidence that this crash might be due to multithreading issues. The reason is that when inspecting memory at %rdx (edge) and I see that a pointer at %rdx+0x48 (edge->first) doesn't match with a pointer in %rcx (edge->first taken at the start of the loop). Besides, freetype code can never produce %rax == 0 at the crash location, this would only happen if af_latin_hints_compute_edges is called concurrently on the same hints structure, which causes pointers to change for segments that are processed in another thread. Best of luck, ThreadStackTrace.txt shows exactly that, two threads are in af_latin_hints_compute_edges with same parameters!

Now the real question is which application or library is actually violating thread-safety here...

[Alexey Borzenkov (snaury)]

I looked at the implementation of cairo-ft-font.c and it seems that there are huge multi-threading violations in cairo. The reason is that FreeType API documentation clearly states:

In multi-threaded applications, make sure that the same FT_Library object or any of its children doesn't get accessed in parallel.

Cairo initializes FT_Library for its font map and there's a lock for that, however it's only used for font map manipulations, the actual FT_ library calls are completely unprotected with that lock, although they should be! What adds to the injury is that cairo-ft-font.c even has this comment on one of its functions:

You must be careful when using this function in a library or in a
threaded application, because freetype's design makes it unsafe to
call freetype functions simultaneously from multiple threads, (even
if using distinct FT_Face objects)

Too bad they don't follow their own advice, and no wonder compiz is crashing like that. Can somebody contact upstream about this issue and make them aware of it?

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Shahar Or (mightyiam)]

Brilliant work, Alexey.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Steve Langasek (vorlon)]

This crash happened with libfreetype6 2.4.12-0ubuntu1 installed. Is it still reproducible with 2.5.2-1ubuntu2, the version in Ubuntu 14.04?

[Steve Langasek (vorlon)]

actually, based on the previous comment it looks lke it almost certainly would still occur with new freetype since these interfaces are not thread-safe; seems that this need to be fixed in cairo (or in compiz applying its own mutexes).

[Alexey Borzenkov (snaury)]

Steve, the latest duplicate shows 2.5.2-1ubuntu2:

https://bugs.launchpad.net/bugs/1304763

Is it possible to get some input from cairo maintainers?

[Sebastien Bacher (seb128)]

> Is it possible to get some input from cairo maintainers?

There is not really an "Ubuntu cairo maintainer", the bug should be report to upstream on https://bugs.freedesktop.org/enter_bug.cgi?product=cairo

[Alberto Salvia Novella (es20490446e)]

I looked at the implementation of cairo-ft-font.c and it seems that there are huge multi-threading violations in cairo. The reason is that FreeType API documentation clearly states:

In multi-threaded applications, make sure that the same FT_Library object or any of its children doesn't get accessed in parallel.

Cairo initializes FT_Library for its font map and there's a lock for that, however it's only used for font map manipulations, the actual FT_ library calls are completely unprotected with that lock, although they should be! What adds to the injury is that cairo-ft-font.c even has this comment on one of its functions:

You must be careful when using this function in a library or in a
threaded application, because freetype's design makes it unsafe to
call freetype functions simultaneously from multiple threads, (even
if using distinct FT_Face objects)

[Alberto Salvia Novella (es20490446e)]

It severely affects packages beyond the one that causes the flaw.

[Freedesktop (freedesktop)]

*** Bug 81874 has been marked as a duplicate of this bug. ***

[Freedesktop (freedesktop)]

Looks like copying thread-local storage macros from pixman-compiler.h and using them to use per-thread FT_Library might easily get most of this resolved. I can take a look if no one else steps in.

[Marek Kašík (mkasik)]

(In reply to Behdad Esfahbod from comment #4)
> Looks like copying thread-local storage macros from pixman-compiler.h and
> using them to use per-thread FT_Library might easily get most of this
> resolved. I can take a look if no one else steps in.

Hi Behdad,

had you time to look at this already?

Regards

Marek

[Freedesktop (freedesktop)]

So, I ended up trying to fix this in FreeType. Have made huge progress so far. Here's a tree:

  https://github.com/behdad/freetype/commits/ftthread

And here's a standalone test:

  https://github.com/behdad/ftthread

There's still some more work to do. I can't yet understand this crash for example:

https://bugzilla.redhat.com/show_bug.cgi?id=1165471

[Freedesktop (freedesktop)]

I now have a complete patchset up here:

  https://github.com/behdad/freetype/commits/ftthread

Will be sending upstream soon.

[Freedesktop (freedesktop)]

Done. See:
http://<email address hidden>/msg06758.html

[Freedesktop (freedesktop)]

Patchset is now upstream in FreeType.

[Marek Kašík (mkasik)]

(In reply to Behdad Esfahbod from comment #9)
> Patchset is now upstream in FreeType.

Thank you very much for all the effort you've put into this!

[Shahar Or (mightyiam)]

Yes, thank you.

[Tarjei Huse (tarjei-huse)]

Will this fix be released for 14.04, or do I have to wait for 15.04?

[Marco Trevisan (Treviño) (3v1n0)]

Tarjei: as always it happens with important bugs, they are first released to the development version and after that it has been verified there, it will be backported to trusty as well through an SRU (code in place for that is already there).

[Launchpad Janitor (janitor)]

This bug was fixed in the package freetype - 2.5.2-2ubuntu2

---------------
freetype (2.5.2-2ubuntu2) vivid; urgency=medium

  * Added patchset to fix multithread violations, LP: #1199571
    - debian/patches-freetype/multi-thread-violations.patch
 -- Marco Trevisan (Trevino) <email address hidden> Fri, 23 Jan 2015 03:23:18 +0100

[Brian Murray (brian-murray)]

This bug report is missing Stable Release Update information as discussed at http://wiki.ubuntu.com/StableReleaseUpdates. A test case isn't strictly necessary for this bug report as there is at least one crash report at the Ubuntu Error Tracker we can use to make sure the version of the package being SRU'ed isn't appearing there.

[Chris J Arges (arges)]

Hello Brad, or anyone else affected,

Accepted freetype into trusty-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/freetype/2.5.2-1ubuntu2.3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Marco Trevisan (Treviño) (3v1n0)]

Everything work as expected with new freetype 2.5.2-1ubuntu2.3

This is what was happening with freetype 2.5.2-1ubuntu2.2
time while ./ftthread /usr/share/fonts/truetype/ubuntu-font-family/Ubuntu-R.ttf 40 10000 150 4; do echo -n '.'; done
FT_Load_Glyph failed: FT_Err_Invalid_Reference: invalid reference.
Segmentation fault (core dump created)

real 0m0.376s

This is instead what I'm getting with 2.5.2-1ubuntu2.3:

time while ./ftthread /usr/share/fonts/truetype/ubuntu-font-family/Ubuntu-R.ttf 40 10000 150 4; do echo -n '.'; done
...........................................

real 32m0.545s

[Launchpad Janitor (janitor)]

This bug was fixed in the package freetype - 2.5.2-1ubuntu2.3

---------------
freetype (2.5.2-1ubuntu2.3) trusty; urgency=medium

  * Added patchset to fix multithread violations, LP: #1199571
    - debian/patches-freetype/multi-thread-violations.patch
 -- Marco Trevisan (Trevino) <email address hidden> Fri, 23 Jan 2015 03:38:04 +0100

[Adam Conrad (adconrad)]

The verification of the Stable Release Update for freetype has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.