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

[Albert Astals Cid]

A Divendres, 15 de gener de 2010, mpsuzuki at hiroshima-u.ac.jp va escriure:
> Hi,
> 
> 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, 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)

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.

Unless someone proves me wrong and i'll be happy to fix it :D

Albert

> 
> In the creation of GlobalParams object, a fontconfig
> configuration object is allocated by calling
> FcConfigGetCurrent(), aslike:
> 
>     554 GlobalParams::GlobalParams(const char *customPopplerDataDir)
>     555   : popplerDataDir(customPopplerDataDir)
>     556 {
>     557   UnicodeMap *map;
>     558   int i;
>     559
>     560 #ifndef _MSC_VER
>     561   FcInit();
>     562   FCcfg = FcConfigGetCurrent();
>     563 #endif
> 
> The object created by FcConfigGetCurrent() could be
> deleted by calling FcConfigDestroy(). In the deletion
> of GlobalParams object:
> 
>     796 GlobalParams::~GlobalParams() {
>     797   freeBuiltinFontTables();
>     798
>     799   delete macRomanReverseMap;
>     800
>     801   delete baseDir;
>     802   delete nameToUnicode;
>     803   deleteGooHash(cidToUnicodes, GooString);
>     804   deleteGooHash(unicodeToUnicodes, GooString);
>     805   deleteGooHash(residentUnicodeMaps, UnicodeMap);
>     806   deleteGooHash(unicodeMaps, GooString);
>     807   deleteGooList(toUnicodeDirs, GooString);
>     808   deleteGooHash(displayFonts, DisplayFontParam);
>     809 #ifdef _WIN32
>     810   delete winFontList;
>     811 #endif
>     812   deleteGooHash(psFonts, PSFontParam);
>     813   deleteGooList(psNamedFonts16, PSFontParam);
>     814   deleteGooList(psFonts16, PSFontParam);
>     815   delete textEncoding;
>     816   deleteGooList(fontDirs, GooString);
>     817
>     818   GooHashIter *iter;
>     819   GooString *key;
>     820   cMapDirs->startIter(&iter);
>     821   void *val;
>     822   while (cMapDirs->getNext(&iter, &key, &val)) {
>     823     GooList* list = (GooList*)val;
>     824     deleteGooList(list, GooString);
>     825   }
>     826   delete cMapDirs;
>     827
>     828   delete cidToUnicodeCache;
>     829   delete unicodeToUnicodeCache;
>     830   delete unicodeMapCache;
>     831   delete cMapCache;
>     832
>     833 #ifdef ENABLE_PLUGINS
>     834   delete securityHandlers;
>     835   deleteGooList(plugins, Plugin);
>     836 #endif
>     837
>     838 #if MULTITHREADED
>     839   gDestroyMutex(&mutex);
>     840   gDestroyMutex(&unicodeMapCacheMutex);
>     841   gDestroyMutex(&cMapCacheMutex);
>     842 #endif
>     843
>     844 }
> 
> FcConfigDestroy() is not called, and FCcfg could be leaked.
> So I propose to insert FcConfigDestroy() aslike:
> 
> diff --git a/poppler/GlobalParams.cc b/poppler/GlobalParams.cc
> index 2813b98..ba93a5f 100644
> --- a/poppler/GlobalParams.cc
> +++ b/poppler/GlobalParams.cc
> @@ -840,6 +840,11 @@ GlobalParams::~GlobalParams() {
>    gDestroyMutex(&unicodeMapCacheMutex);
>    gDestroyMutex(&cMapCacheMutex);
>  #endif
> +
> +#ifndef _MSC_VER
> +  FcConfigDestroy(FCcfg);
> +#endif
> +
>  }
> 
>  //------------------------------------------------------------------------
> 
> However, I'm not sure if it should be deleted in
> earlier part. Please give me comment.
> 
> Regards,
> mpsuzuki
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>