Opened 2 years ago

Closed 2 years ago

#10217 closed defect (fixed)

Possible memory leak for Image::writePNG

Reported by: dafioram Owned by: csnover
Priority: low Component: --Other--
Keywords: Cc:
Game:

Description

scummvm: a7479b4f5beb83e75379f868d5c19c23625eabe6

==26769== 344 bytes in 1 blocks are definitely lost in loss record 2,337 of 2,409
==26769==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26769==    by 0x5D2BEE2: png_create_info_struct (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.28.0)
==26769==    by 0x2A20B7: Image::writePNG(Common::WriteStream&, Graphics::Surface const&, bool) (png.cpp:290)
==26769==    by 0x2703D6: SurfaceSdlGraphicsManager::saveScreenshot(char const*) (surfacesdl-graphics.cpp:1434)
==26769==    by 0x27466A: SurfaceSdlGraphicsManager::notifyEvent(Common::Event const&) (surfacesdl-graphics.cpp:2666)
==26769==    by 0x3AF1D1: Common::EventDispatcher::dispatchEvent(Common::Event const&) (EventDispatcher.cpp:144)
==26769==    by 0x3AEC08: Common::EventDispatcher::dispatch() (EventDispatcher.cpp:61)
==26769==    by 0x25D9A8: DefaultEventManager::pollEvent(Common::Event&) (default-events.cpp:93)
==26769==    by 0x1AA8A0: Mohawk::MohawkEngine_Riven::doFrame() (riven.cpp:208)
==26769==    by 0x1AA7EF: Mohawk::MohawkEngine_Riven::run() (riven.cpp:196)
==26769==    by 0x1643D8: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:263)
==26769==    by 0x1655F7: scummvm_main (main.cpp:529)

Change History (7)

comment:1 by wjp, 2 years ago

Summary: MOHAWK: Riven: Possible memory leak for doFramePossible memory leak for Image::writePNG

comment:2 by wjp, 2 years ago

Component: Engine: Mohawk--Other--
Game: Riven

comment:3 by dafioram, 2 years ago

I was originally thinking adding a

png_destroy_write_struct(&pngPtr, &endInfo);

after line 317 would make sense. But it looks like it may make the most sense to just remove:

	png_infop endInfo = png_create_info_struct(pngPtr);
	if (!endInfo) {
		png_destroy_write_struct(&pngPtr, &infoPtr);
		return false;
	}

from Image::writePNG. I think the endInfo creation in writePNG is in there because it was copied from PNGDecoder::loadStream which uses endInfo as apart of png_destroy_read_struct(&pngPtr, &infoPtr, &endInfo); in line 242. Since endInfo is not used in writePNG it can probably be removed.

comment:4 by wjp, 2 years ago

Actually loadStream doesn't use endInfo either. It could be used by passing it to png_read_end, but this isn't done.

comment:5 by dafioram, 2 years ago

It looks like it is still needed to destroy the read structure, line 242,

png_destroy_read_struct(&pngPtr, &infoPtr, &endInfo);

I don't know what adding it to png_read_end would do, png_read_end is currently passing in NULL and endInfo isn't NULL, line 126.

comment:6 by dafioram, 2 years ago

Fixed by PR1028.

comment:7 by csnover, 2 years ago

Owner: set to csnover
Resolution: fixed
Status: newclosed

Thanks for your help finding and fixing this! A patch for this issue has been added in commit 194984de2f8deca747b20e646cda9d54705f1723 and will be available in daily builds 1.10.0git-4974 and higher.

Note: See TracTickets for help on using tickets.