[vlc-devel] [PATCH] gui/qt: pictureflow: clear cache on destruction

[Filip Roséen]

Hi Rémi,

On 2017-03-01 16:16, Rémi Denis-Courmont wrote:

> On March 1, 2017 10:52:03 AM GMT+02:00, "Filip Roséen" <filip at atch.se> wrote:
> >PictureFlowSoftwareRenderer::PictureFlowSoftwareRenderer():
> > PictureFlowSoftwareRenderer::~PictureFlowSoftwareRenderer()
> > {
> >     buffer = QImage();
> >+
> >+    for( QHash<QString, QImage*>::const_iterator
> >+         it = cache.constBegin(); it != cache.constEnd(); ++it )
> >+    {
> >+        delete it.value();
> >+    }
> >+
> >     cache.clear();
> >     delete blankSurface;
> > }
> 
> I do not know if the patch is valid or not. But "indirect" leak
> implies that the container is leaked. (And so, that a double free
> could potentially occur if the container leak is fixed.)

 1. The container is a direct member of `PictureFlowSoftwareRenderer`,
    and as `PictureFlowSoftwareRenderer::~PictureFlowSoftwareRenderer()`
    is called, so will the destructor of the container.

 2. The destructor of `QHash<QString, QImage*>` will not take care of
    cleaning up after `T*`.

> Isn't there a "direct" leak?

AFAICT, not related to the leak which this patch is set to address,
below is the full log (from a session where the current commit is
reverted).

 - https://gist.github.com/anonymous/80ad4c56d6d3222a2a1937a7e9f5ca5b

I think there is some part of either the "qt plugin" responsible for
the image handling in question, or the `QPainter` itself (with
automatic storage duration in our implementaiton), that stores a
pointer to the `QImage` for which it is created, without considering
it to have ownership of said pointer (somehow fooling the sanitizer
into thinking that the leak is indirect).

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170301/021276fa/attachment.html>