Fontconfig - Jan 2006 - Missing FcValueSave() in FcConfigEvaluate() (KDE bug #119108)

Hello,

 please review the attached patch. It should fix a fontconfig crash triggered 
by a Qt patch that should improve font loading performance 
(FcFontSort->FcFontMatch, but that should be actually irrelevant). A Valgrind
log for the crash is attached, it''s a double free.

 Now, I actually don''t have any simple testcase or a good understanding
of
those strange things done in fontconfig, but I have the patch :). As the 
patch seems to be rather simple and obvious, I hope we can skip this 
complicated step.

 If I''m getting it right, FcConfigEvaluate() always allocates its
result,
which should be somewhen later freed. It itself does so e.g. in the 
FcOpOr..FcOpDivide cases, where it recursively calls itself. In the FcOpField 
case it calls FcPatternGet(), which however doesn''t seem to create a
copy of
the value, it only calls FcValueCanonicalize() and assigns the value. This 
specific crash seems to happen only for type FcTypeMatrix.

 If the patch is ok, please apply it. Otherwise I''ll try to elaborate
more on
the problem or try to create a testcase.

-- 
Lubos Lunak
KDE developer
---------------------------------------------------------------------
SuSE CR, s.r.o.  e-mail: l.lunak@suse.cz , l.lunak@kde.org
Drahobejlova 27  tel: +420 2 9654 2373
190 00 Praha 9   fax: +420 2 9654 2374
Czech Republic   http://www.suse.cz/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fccfg.c.patch
Type: text/x-diff
Size: 364 bytes
Desc: not available
Url :
http://lists.freedesktop.org/archives/fontconfig/attachments/20060102/e5a586e7/fccfg.c.bin
-------------- next part --------------
==19365== Invalid free() / delete / delete[]
==19365==    at 0x1B9003C3: free (in /usr/lib/valgrind/vgpreload_memcheck.so)
==19365==    by 0x1B92070A: FcMatrixFree (fcmatrix.c:52)
==19365==    by 0x1B922642: FcValueListDestroy (fcpat.c:160)
==19365==    by 0x1B911923: FcConfigDel (fccfg.c:1216)
==19365==    by 0x1B911A00: FcConfigPatternDel (fccfg.c:1249)
==19365==    by 0x1B911F3E: FcConfigSubstituteWithPat (fccfg.c:1423)
==19365==    by 0x1B91F7D4: FcFontRenderPrepare (fcmatch.c:501)
==19365==    by 0x1B91FD7A: FcFontSetMatch (fcmatch.c:696)
==19365==    by 0x1B91FE15: FcFontMatch (fcmatch.c:718)
==19365==    by 0x42E158AD: XftFontMatch (in /usr/X11R6/lib/libXft.so.2.1.2)
==19365==    by 0x1BEC6D8D: loadFontConfigFont(QFontPrivate const*, QFontDef
const&, QFont::Script) (qfontdatabase_x11.cpp:1895)
==19365==    by 0x1BEC753B: QFontDatabase::findFont(QFont::Script, QFontPrivate
const*, QFontDef const&, int) (qfontdatabase.cpp:981)
==19365==  Address 0x1C3F32D0 is 0 bytes inside a block of size 32
free''d
==19365==    at 0x1B9003C3: free (in /usr/lib/valgrind/vgpreload_memcheck.so)
==19365==    by 0x1B92070A: FcMatrixFree (fcmatrix.c:52)
==19365==    by 0x1B922328: FcValueDestroy (fcpat.c:78)
==19365==    by 0x1B910B8E: FcConfigEvaluate (fccfg.c:962)
==19365==    by 0x1B911509: FcConfigValues (fccfg.c:1113)
==19365==    by 0x1B911D83: FcConfigSubstituteWithPat (fccfg.c:1362)
==19365==    by 0x1B91F7D4: FcFontRenderPrepare (fcmatch.c:501)
==19365==    by 0x1B91FD7A: FcFontSetMatch (fcmatch.c:696)
==19365==    by 0x1B91FE15: FcFontMatch (fcmatch.c:718)
==19365==    by 0x42E158AD: XftFontMatch (in /usr/X11R6/lib/libXft.so.2.1.2)
==19365==    by 0x1BEC6D8D: loadFontConfigFont(QFontPrivate const*, QFontDef
const&, QFont::Script) (qfontdatabase_x11.cpp:1895)
==19365==    by 0x1BEC753B: QFontDatabase::findFont(QFont::Script, QFontPrivate
const*, QFontDef const&, int) (qfontdatabase.cpp:981)

On Mon, 2 Jan 2006, Lubos Lunak wrote:
> If I''m getting it right, FcConfigEvaluate() always allocates its
result,
> which should be somewhen later freed. It itself does so e.g. in the
> FcOpOr..FcOpDivide cases, where it recursively calls itself. In the
FcOpField
> case it calls FcPatternGet(), which however doesn''t seem to create
a copy of
> the value, it only calls FcValueCanonicalize() and assigns the value. This
> specific crash seems to happen only for type FcTypeMatrix.
Looks good to me.  FcValueCanonicalize does not create an extra copy, 
FcValueSave is the right thing to do here.  I''ve committed this patch 
(since it''s short).

I still don''t have good computer access, so I haven''t been
able to review
other patches yet.  Sorry.

pat

On Mon, 2 Jan 2006, Lubos Lunak wrote:
>
>  Hello,
>
>  please review the attached patch. It should fix a fontconfig crash
triggered
> by a Qt patch that should improve font loading performance
> (FcFontSort->FcFontMatch, but that should be actually irrelevant). A
Valgrind
> log for the crash is attached, it''s a double free.

I''m curious about the original patch, since fontconfig
(FcFontSort) is taking around 30% of time in Firefox, and quite a
lot in other scenarios too...

--behdad
http://behdad.org/

"Commandment Three says Do Not Kill, Amendment Two says Blood Will
Spill"
	-- Dan Bern, "New American Language"

On Tuesday 03 January 2006 00:44, Behdad Esfahbod wrote:
> On Mon, 2 Jan 2006, Lubos Lunak wrote:
> >  Hello,
> >
> >  please review the attached patch. It should fix a fontconfig crash
> > triggered by a Qt patch that should improve font loading performance
> > (FcFontSort->FcFontMatch, but that should be actually irrelevant).
A
> > Valgrind log for the crash is attached, it''s a double free.
>
> I''m curious about the original patch, since fontconfig
> (FcFontSort) is taking around 30% of time in Firefox, and quite a
> lot in other scenarios too...
 The patch is at 
http://websvn.kde.org/*checkout*/branches/qt/3.3/qt-copy/patches/0066-fcsort2fcmatch.patch
.
Because of whitespace changes it looks more complicated than it really is, as 
it is just trying FcFontMatch() before going the noticeably more expensive 
FcFontSort() way.

 Basically, in that function Qt creates FcPattern that describes the font it 
wants, then does FcFontSort() and starts checking the results until it finds 
one that fully satisfies its additional requirements (as I''m not fonts
expert
I have actually no clue what that code really does). And since I noticed that 
with a reasonable fonts installation the very first match is already enough, 
it should be sufficient to just get that very first match using FcFontMatch() 
instead of finding all suitable fonts with the more expensive FcFontSort(), 
which should be only used as a fallback.

 I''m not sure if the same could be done in Firefox though - this is Qt3
and in
Qt4 it won''t be possible to do this, as Qt4 seems to try to combine
more
fonts into one when just one doesn''t provide all the required
characters.
There FcFontSort() will be still needed :-/. I''m not yet sure what to
do with
that.

-- 
Lubos Lunak
KDE developer
---------------------------------------------------------------------
SuSE CR, s.r.o.  e-mail: l.lunak@suse.cz , l.lunak@kde.org
Drahobejlova 27  tel: +420 2 9654 2373
190 00 Praha 9   fax: +420 2 9654 2374
Czech Republic   http://www.suse.cz/