[cairo] [cairo-commit] 10 commits - configure.in src/cairo-arc.c src/cairo-pattern.c src/cairo-scaled-font.c src/cairo-scaled-font-private.h src/cairo-xlib-display.c src/cairo-xlib-private.h src/cairo-xlib-surface.c test/cairo-test.c test/degenerate-arc.c test/degenerate-arc-ps-ref.png test/degenerate-arc-ref.png test/.gitignore test/invalid-matrix.c test/Makefile.am

[Chris Wilson]

On Thu, 2008-05-08 at 11:53 -0400, Behdad Esfahbod wrote:
> On Wed, 2008-05-07 at 00:22 -0700, Chris Wilson wrote:
> > 
> > +       to_free = glyphset_info->pending_free_glyphs;
> > +       if (to_free != NULL &&
> > +           to_free->glyph_count == ARRAY_LENGTH
> > (to_free->glyph_indices))
> > +       {
> > +           status = _cairo_xlib_display_queue_work
> > (font_private->display,
> >                     (cairo_xlib_notify_func)
> > _cairo_xlib_render_free_glyphs,
> > -                   arg,
> > +                   to_free,
> >                     free);
> > -           if (status) {
> > -               /* XXX cannot propagate failure */
> > -               free (arg);
> > +           /* XXX cannot propagate failure */
> > +           if (status)
> > +               free (to_free);
> > +
> > +           to_free = glyphset_info->pending_free_glyphs = NULL;
> > +       }
> > +
> > +       if (to_free == NULL) {
> > +           to_free = malloc (sizeof
> > (cairo_xlib_font_glyphset_free_glyphs_t));
> > +           if (to_free == NULL) {
> > +               _cairo_error_throw (CAIRO_STATUS_NO_MEMORY);
> > +               return; /* XXX cannot propagate failure */
> >             }
> 
> Here you can avoid freeing the to_free array and allocating it again...
> 
> Also this definitely doesn't look right:
> 
> 
> > +           if (status)
> > +               free (to_free);
> > +
> > +           to_free = glyphset_info->pending_free_glyphs = NULL;
> 
> Aren't you leaking to_free in case of success??

Here we are trying to append the array of expired glyphs to the work
queue and in doing so transfer ownership of the array to the work queue
- which will call our destructor (free) when it has completed. So if
that fails, we free the array and forget about all the glyphs we wanted
to release. Fortunately, this is just a minor bookkeeping error in our
monitoring of server-side resources and not a gargantuan glyph leak!
-- 
Chris Wilson