[poppler] A memory leak in GlobalParams::~GlobalParams()?

[Albert Astals Cid]

A Dijous, 21 de gener de 2010, mpsuzuki at hiroshima-u.ac.jp va escriure:
> Hi,
> 
> On Wed, 20 Jan 2010 21:11:09 +0000
> 
> Albert Astals Cid <aacid at kde.org> wrote:
> >A Divendres, 15 de gener de 2010, mpsuzuki at hiroshima-u.ac.jp va escriure:
> >> I had ever reported a possibility of memory leak in
> >> CairoFreeTypeFont::create(), then, I found another
> >> possibility of memory leak in GlobalParams::~GlobalParams().
> >
> >I am not sure of that, FcConfigGetCurrent not always creates an object,
> 
> Oh, I'm sorry for missing the possibility that other
> part of software has already initialized "_fcConfig"
> and poppler just refers it via FcConfigGetCurrent().
> Thank you very much.
> 
> >as it can return you an already existing object, so deleting
> >on exit doesn't really seem wise as you could be removing
> >the object from underneath the feet of whoever else created
> >it (Qt, Pango, Cairo, etc)
> 
> I see. Checking fontconfig source, I found FcConfig
> has a reference counter, and FcConfigDestroy() checks
> it so that the client can prevent freeing the object
> in use.
> 
> fontconfig/src/fccfg.c
> 
>     233 void
>     234 FcConfigDestroy (FcConfig *config)
>     235 {
>     236     FcSetName   set;
>     237     FcExprPage  *page;
>     238
>     239     if (--config->ref > 0)
>     240         return;
>     241
>     242     if (config == _fcConfig)
>     243         _fcConfig = 0;
>     ...
> 
> However, FcConfigGetCurrent() does not increment the
> reference counter when it returns existing object.
> I will ask the correct procedure how to obtain FcConfig
> and free it in fontconfig mail list.

Yeah, i saw that too, thanks for following this up with them.

> 
> >So as you're supposed to only create one GlobalParams per
> >execution i don't think it's a problematic leak and will
> >vote for leaving it as it is.
> 
> Do you think it's a leak but not so serious to apply
> my wrong patch?  I understood my patch was wrong and
> must be reworked. Anyway, I attached valgrind logs of
> pdftotext, original one (LOG-20100121a.txt) and patched
> one (LOG-20100121b.txt).

I mean it is not a "real" leak, let me explain that, each application that 
uses poppler has (or should have) exactly one GlobalParams object, that gets 
constructed at the begining of the application and destroyed at the end. So 
"fixing" that leak is merely a "let's make it right" issue, i do not maen that 
it is not important, but i also mean we can perfectly live without this fixed 
forever and noone will notice.

Albert

> 
> Regards,
> mpsuzuki
>