Re: [XForms] Bug Fix - fonts.c

[Sunny]

Hi,

I just re-compiled and tested your modified fonts.c with Basic.h. It's good.

Yeah, I agree that re-writing core parts of the library could take a
some time, and there are not enough users/interest. I don't see XForms
attracting a lot of people either, unless something like wayland
support is added - which could be interesting as it's new and shiny.

Anyway, I like the library the way it is now - fast, small and mostly
functional.

Happy coding.
Regards,

Sunny

On 13/04/2014, Jens Thoms Toerring <address@hidden> wrote:
> 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
>