hughsie / appstream-glib

This library provides objects and helper methods to help reading and writing AppStream metadata.
GNU Lesser General Public License v2.1
64 stars 103 forks source link

appstream-builder crashes when generating metadata for Mageia Cauldron x86_64 core/release #177

Closed Conan-Kudo closed 5 years ago

Conan-Kudo commented 7 years ago

In Mageia bug 18669 (the bug for creating AppStream data), one of our sysadmins discovered that appstream-builder 0.6.13 segfaults when processing the metadata with 8 threads.

The output/crash log: appstream-builder-crashlog.txt

The packages mentioned in the crash log:

I can't reproduce with my current system (I don't have enough cores to match the 8 thread command), but maybe you have some idea?

Conan-Kudo commented 7 years ago

It wound up only being successful with one thread on the infrastructure server. :(

Conan-Kudo commented 7 years ago

I've filed a separate downstream bug for this issue: Mageia bug 21207

hughsie commented 7 years ago

I see a libfontconfig crash in the backtrace. This was happening for Fedora, and so much so I've started running the extraction in single-thread mode. Since then, no crash. Fonts really are awful.

Conan-Kudo commented 7 years ago

Erk. And we were just talking about how bad fontconfig is at the sprint a couple weeks back...

grumbles

I hate fonts...

pterjan commented 7 years ago

I doubt this will help, but calling FcConfigAppFontClear before FcConfigDestroy at https://github.com/hughsie/appstream-glib/blob/master/libappstream-builder/plugins/asb-plugin-font.c#L693 is not needed as FcConfigDestroy calls FcFontSetDestroy on all the font sets, including app.

More interestingly I have doubts with the comment on https://cgit.freedesktop.org/fontconfig/tree/src/fccfg.c#n36

It stores the "current config" which is shared between threads, for example this call stores the config for the current thread into that static variable https://github.com/hughsie/appstream-glib/blame/master/libappstream-builder/plugins/asb-plugin-font.c#L596

pterjan commented 7 years ago

To expand on my previous comment as this is not obvious, Thread1 FcConfigSetCurrent(C1) -> C1 is current Thread2 FcConfigSetCurrent(C2) -> C2 is current Thread2 FcConfigDestroy(C2) -> no current config

So calling FcConfigSetCurrent doesn't guarantee there will be a current config later while using C1 This shouldn't be a problem as config is used everywhere so the default one should not matter, but I believe it's better to kill that call which is misleading

ximion commented 7 years ago

@pterjan If I recall correctly, I think I tried exactly that in appstream-generator, which made crashes rare, but they still occasionally happened for reasons I didn't investigate much further (I was so annoyed at that time, that I just crammed the font rendering into a synchronized block, which is the way of the D language to ensure a section of code is run exclusively. As a consequence, if you have many fonts, generating AppStream data now takes a bit longer)

hughsie commented 5 years ago

We're all single-threaded now. :(