[cairo] Memory leak with font variations

[Behdad Esfahbod]

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/54fc810e/attachment.htm>