Re: [XForms] Bug Fix - fonts.c

[Jens Thoms Toerring]

Hi,

On Fri, Apr 11, 2014 at 08:32:21PM +0530, Sunny wrote:
> >So it crashed for you too, but in a different way? I am using the
> >-stable library so it crashed on the 10th or 13th call to
> >fl_set_object_lsize().
> 
> Okay, I actually found the bug. The actual culprit for the double free
> were the below lines:
> 
>         fs = k == -1 ? ( flx->fs ? flx->fs : defaultfs ) : flf->fs[ k ];
> 
>         /* If we did not get it this time, we won't get it next time either,
>            so replace it with whatever we found  */
> 
>         flf->size[ flf->nsize ] = size;
>         flf->fs[ flf->nsize ] = fs;
>         flf->nsize++;
> 
> As you can see, this basically results in two pointer references for
> the same XFontStruct. flf->fs[k] and flf->fs[flf->nsize] refer to the
> same object.
> 
> Later on, the library frees them by looping from 0 to flf->nsize. So
> it frees flf->fs[k], which is correct but then also attempts to free
> flf->fs[ flf->nsize ] which results in the crash (as it refers to
> flf->fs[k].

Yes, exactly

> The below is the diff I got from git - it looks odd so let me know if
> I need to repost it. I basically added a #if 0... #endif to the wrong
> code and introduced some safe checks. The last ditch attempt for fs is
> now this:
>         fs = flx->fs ? flx->fs : defaultfs;

I had another look at your patch and decided to do it a bit dif-
ferently. The reason is that if the font isn't cached each new
attempt to use it will result in another lengthy search for it
(when it gets asked for once it probably has been set for some
label and will be needed again and again in future redraws).

What we need is a way to mark a replacement font so that it's
clear it is just a copy of a font already loaded for a different
size (or is the very basic default font) and never let it be
passed to XFreeFont(), which results in either a segmentation
fault or a X error. Since there is no extra room for a flag
I now set the stored size for it to the negative of the re-
quetsed size (taking care to catch attempts to load a font
with a negative or zero size, which doesn't make sense).
And then XFreeFont() only is called for fonts marked with
a positive size.

Concerning the caching: I can't say if it's really necessary
nowadays, I haven't run any tests (and that would be a bit dif-
ficult, since the results will probably depend a lot on how
the library is used). What I'm rather sure about is that this
part is used very often - whenever an object needs to be re-
drawn it will be called. And there are programs with thou-
sands pf objects in a single form, so anything that speeds
up redraws (which can happen at a high rate) tends to help.
So I would think at least twice before throwing out the ca-
ching code.

I also have added your fl_get_font_name() function.

The whole font handling stuff is rather unpleasant. If an attempt
should be made to use TTF fonts it definitelu should be rewrit-
ten. I don't all that stuff with fixes size arrays (be it for the
number of fonts that can be used, 48 at the moment, be it the
number of cached sizes, 10). I also would consider adding a function
that allows to remove fonts.

I haven't really looked into what is involved when using
TTF fonts. But whatever it is I would consider making that
something that's optional, i.e. can be switched on or off
during compilation. The reason is that it requires an extra
library which may not be available everywhere.

But I guess that it will be quite an effort to get it implemen-
ted: as far as I can see drawing e.g. string requires drawing
basically bitmaps for each and every characters in the string.
So all the existing routines for drawing strings (which are
rather convoluted, to say the least) would have to be adapted.

And when one introduces TTF fonts it would seem to be a bit
weired not also to start supporting UTF8, but which would
require an even larger amont of changes! It probably would
mean more or less a rewrite of large parts of the library.
And I'm not sure if there's enough interest left in XForms
to justify spending the amount of time required to do that
properly. Perhaps it then would be better to start from
scratch and get rid of all the cruft the library has
aquired over all the years of its existence. But who'd
have the time to do that?

                      Best regards, Jens

PS: I append the new file Basic.h (to go into the lib/include
    dir) and the fonts.c file. When you conform that it works
    now I'll add that to the next development release.
-- 
  \   Jens Thoms Toerring  ________      address@hidden
   \_______________________________      http://toerring.de

Basic.h
Description: Text Data

fonts.c
Description: Text Data