[cairo] Memory leak with font variations

Sat Jun 13 21:03:07 UTC 2020

Thanks!
I will try to test it soon.
As I needed a quick and working solution for a bigger project I am working
on, for now I simply replaced in my personal Cairo branch all
re-assignments of the font variation string with a direct copy of the
pointer variable, as the pointer to the string will always exist in my
program. But I will check if your patch solves the problem also for the
dynamic assignment case.
Thanks again,
Toby

On Sat, Jun 13, 2020 at 10:44 PM Behdad Esfahbod <behdad at behdad.org> wrote:

> You are absolutely right.  It's a mess.
>
> cairo_font_options_t used to be a plain old struct that didn't own
> anything.  The ->get_font_options() unfortunately was designed to take a
> *options that it assumed didn't need cleanup and just plain wrote to it.
>
> When we added variations, all those assumptions broke.  I've given it a
> try to clean up:
>
>
> https://github.com/behdad/cairo/commit/53cffff1578e4bbd1fa4d8f730c6a963617572bb
>
> I think I got it all right but am not completely sure.  I think what I did
> is the best approach given the mess we are in.
>
> Please review and test.  It compiles but I haven't tested at all.  I also
> wrote the patch on a very old checkout of mine then rebase on master.  I
> missed any code in between that would need the same treatment but can't do
> another review right now.
>
> Hope someone can take it from there.
>
> Thanks,
> behdad
>
> PS. I also found this mirror is stalled and confusing:
>
>   https://github.com/freedesktop/cairo/commits/master
>
> On Wed, Jun 10, 2020 at 10:39 PM Tobias Fleischer (reduxFX) <
> tobias.fleischer at reduxfx.com> wrote:
>
>> Unfortunately, I just found out it is not the only location where this is
>> happening :(
>> For example cairo_scaled_font_create() also calls
>> _cairo_font_options_init_copy(), which then leads to a leak.
>> I tried to add these lines to _cairo_scaled_font_fini_internal():
>> _cairo_font_options_fini (&scaled_font->options);
>> scaled_font->options.variations = NULL;
>>
>> But that didn't help, I still get leaks because of unmatched malloc/free
>> blocks. Maybe it has to do with the fontmap caching/release count here?
>>
>> Or maybe we shouldn't try to duplicate the variation option string at
>> all, but just copy the pointer and say it is up to the calling application
>> to guarantee its existence?
>> A fixed char array could also be an option but that does sound very hacky.
>>
>> On Thu, Jun 11, 2020 at 7:30 AM Behdad Esfahbod <behdad at behdad.org>
>> wrote:
>>
>>> Thanks Tobias. This definitely makes sense. Thanks for debugging. I'll
>>> try to take a look on computer and fix. Unless, I hope, someone beats me to
>>> that.
>>>
>>> On Wed, Jun 10, 2020, 10:25 PM Tobias Fleischer (reduxFX) <
>>> tobias.fleischer at reduxfx.com> wrote:
>>>
>>>> I think I found a bug concerning non-released memory when using font
>>>> variations.
>>>> I tested against cairo-1.17.2.
>>>>
>>>> The internal function _cairo_gstate_init_copy() is supposed to make a
>>>> deep copy of the fields from one instance/state to another, used for
>>>> example by cairo_save(). It does however call
>>>> _cairo_font_options_init_copy(), which has this line in it:
>>>> options->variations = other->variations ? strdup (other->variations) :
>>>> NULL;
>>>>
>>>> This means that if a font variation string has been set, instead of a
>>>> copy, it will always allocate and use a copy of the string (via strdup),
>>>> which will then never be freed.
>>>> This leads to memory leaks as for example just by calling cairo_save(),
>>>> with each call an additional pointer is created that is never released.
>>>>
>>>> Simple sample code to reproduce:
>>>> cairo_surface_t* surface =
>>>> cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 1920, 1080);
>>>> cairo_t* cr = cairo_create(surface);
>>>> cairo_font_options_t* t = cairo_font_options_create();
>>>> cairo_get_font_options(cr, t);
>>>> cairo_font_options_set_variations(t, "wght=400");
>>>> cairo_set_font_options(cr, t);
>>>> cairo_font_options_destroy(t);
>>>> cairo_save(cr);
>>>> cairo_restore(cr);
>>>> cairo_destroy(cr);
>>>> cairo_surface_destroy(surface);
>>>>
>>>> I think what is missing is a matching free-and-null call in
>>>> _cairo_gstate_fini().
>>>> If I add the following two lines at the beginning
>>>> of_cairo_gstate_fini(), it seems to fix this issue, as every allocated copy
>>>> gets freed again:
>>>> _cairo_font_options_fini (&gstate->font_options);
>>>> gstate->font_options.variations = NULL;
>>>>
>>>> Let me know if this makes sense.
>>>> Cheers,
>>>> Toby
>>>> --
>>>> cairo mailing list
>>>> cairo at cairographics.org
>>>> https://lists.cairographics.org/mailman/listinfo/cairo
>>>>
>>> --
>> cairo mailing list
>> cairo at cairographics.org
>> https://lists.cairographics.org/mailman/listinfo/cairo
>>
>
>
> --
> behdad
> http://behdad.org/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.cairographics.org/archives/cairo/attachments/20200613/58f921b3/attachment-0001.htm>