Topic: Memory overun\leak patch for fba_029728...

[wcoder]

====> The source code link is on the bottom <====

Hi,

After one week of analyses I localize and correct some memory issues...

I don't like command line tool so i made a complete VC2008 project without Perl, script, asm and code generator (proj not include for the moment...), to allow VC full integration, I made some modification isolate with a pragma: _WZ_VC__.
Don’t be afraid, I check that project can be compiled using mingw (vc_compile.bat)

Conf: UNICODE, BUILD_WIN32, LSB_FIRST (under Win7 32bits)

List of modifications:
* Driver impacted
 + "Aliens (World set 2)"
 + "Devil World"
 + "Caveman Ninja (World ver 4)"
 + "Rohga Armor Force (Asia/Europe v5.0)"
 + "Haunted Castle (ver. M)"
 + "Gauntlet (rev 14)"
 + "Asuka & Asuka (World)"
 
* Hardware impacted
 + Midas
 + SNES
 + Z80
 + Taito
 + Yamaha FM sound generator
 + Irem GA20 PCM Sound Chip
 + Philips SAA1099 Sound driver
 + SN76496
 + UPD7759

* System impacted
 + samples.cpp
 + mdi.cpp
 + scrn.cpp
 + sel.cpp
 + epx.cpp
 + xbr.cpp
 + net.cpp

* Code change.
 - Remove dead code.
 + Simplification
 - Remove memory overrun.
 + Adjust #define
 - Remove resource leak.
 - Initialize uninitialized.
 - Remove unused var.


Most common issue:
 char a[2] = { 0, 0 }
memset (a, 0xff, 3); <- hoops we have a problem the size of the array is 2 nor 3
 a[3] = value; <- hoops we have a problem a max is [2] nor a[3]…

char a [MAXPATH];
char b [MAXPATH];

strcpy ( b, a );
strcat (b, “somethings” ) <- hoops we have a problem MAXPATH + sizeof (“somethings”) is greater than MAXPATH… It's this kind of bug that creates crash on rom search mechanism if you have a big hd tree…

Correction for security I don’t adjust new string size to exact correct size, if the corrected size is 18, I put 24, if its 28, I put 32, if it’s 192, I put 256…

I don’t correct yet all memory issue because some one required more analyze to be sure that it’s really a problem…

For resource leak it’s a little more tricky :
According to MSDN, brush created with CreateSolidBrush(0); must be destroy using DeleteObject, so instead of add a mechanism to release resource, I replace the brush creation by a function that return system brush that do not need to be release GetStockObject.

A found a strange memory leak when I play with Metal Slug X it’s so big that it strange:
SZHVC_add and SZHVC_sub (2x256x256) don’t seems to be release, so I add some code to do the job on Z80Exit...
A big part of the code what disable using #if 0, on this code, memory was release, it’s why I found this strange…

If you use my mod don’t forget to add my pseudo on credit ^_^ : “Wizard Coder”…

Are you interest by the vc2008 project, for debugging and for using visual plug-in like “static code analyze” or “memory leak detector” it’s useful…

WCoder

Note 1 : For more details on mod, use diff tool…
Note 2 : After a coding night the gatcha image is not easy to read...
Note 3 : Archive to big : http://www.mediafire.com/file/0tobysc4o2qe2zs/svn.1074.rar – pwd : fba

[kev]

Thanks. I pushed some of these fixes to the SVN, at least the ones I can verify myself.

[lantus]

excellent work.

[wcoder]

Thanks,

I found an other one this weekend who generate stack corruption, it's a variable that is allocated with a size of 8 to store a CRC32 ((32/8*2 = 8 ) + EOS = 9 nor , the EOS was forgotten, the size need to be adjust to 16... If I remember the var is nCr32xxxx [8]...

To simplify port, I add 5 pragma to disable DDraw (DX7), DDSound, Preview image (Png), net and dwapi, the goal is to keep the same code for other platform instead of branch code (there's a lot of different version of FB version on internet that don't share the same version of the code)...

For my use I need to replace all IO (Disk, Input, Graph, and Sound) and the GUI...

I have an other idea, when i look at the driver i see that a lot of them seems to be copy\paste, do you things it will be possible to have less master driver and store the parameters structure on db or external xml data?

For my job I create software using this kind of design, the size of the code is very small (fast compilation) and the software can be extend without compilation (simplify work for newbie)... The bad point is the time need for refactoring existing code but the result will be pretty cool ^_^

WCoder

EOS: End of string: '\0'
GUI: Graphic User Interface.

[Twinaphex]

I have an other idea, when i look at the driver i see that a lot of them seems to be copy\paste, do you things it will be possible to have less master driver and store the parameters structure on db or external xml data?

Adding dependencies like this is a bad idea. I don't want to start baking in XML for the libretro ports (and I have more than 6/7 platforms to support now with more on the way - so this will definitely not be trivial - not to mention it's unwanted and unneeded).

I have an other idea, when i look at the driver i see that a lot of them seems to be copy\paste, do you things it will be possible to have less master driver and store the parameters structure on db or external xml data?

For my job I create software using this kind of design, the size of the code is very small (fast compilation) and the software can be extend without compilation (simplify work for newbie)... The bad point is the time need for refactoring existing code but the result will be pretty cool ^_^

I don't think it's wise to start refactoring FBA's driver system this late in the game - it has more or less been this way ever since the project began as an offshoot of Dave's project and I think it's more or less fine for 'newbies' to wrap their head around - the only thing you need to do is generate some files with Perl scripts and that is it. Hunting down memory leaks is great and definitely appreciated, but the ideas you've suggested so far (adding db dependencies and/or XML dependencies) in terms of refactoring worry me - it will complicate things for people like me who want to make 'slim' builds of FBA without all the additional bloat. It certainly doesn't belong in an emulator core and it's best to decouple any database system from the core itself instead of creating dependencies like this.

[wcoder]

Hi,

The nasty var is char nCrcBoard[8] = ""; located on sel.cpp just replace it by char nCrcBoard[16] = "";

For the xml it's just an idea ^_^

WCoder

[Barry Harris]

Thanks for those. I picked up the extra crc one whilst I was going through sel.cpp so that one is done too.

xml driver definitions are not likely to be a consideration. The whole program takes less than 5 minutes to compile from clean on my machine, which is modern, but certainly not a powerhouse. It would also be a nightmare for bug reporting, etc. as well.

[wcoder]

Hi,

I work with an old Vaio (4-5 years old)... It takes more more more time for me...
An example when I launch static analyzer it take 1h20... It's a demo version, maybe the final version will take less time (it leaves me a lot of time to found missing rom to complete my rom set ^_^...).

Has I write previously the "The bad point is the time need for refactoring existing code..."... So I act: it's a bad idea ... don't spend more time with this...

I have a question, when I select only SNES, the unavailable\available rom list result is empty is it normal or I break something’s ?

WCoder