ledutil.c memory leak?

[headkaze]

I think there is a memory leak in the Mame.dll file which is used in a few applications developed by people on here (including LEDBlinky, CPWizard, Mame Hooker and PluginLCD). Mame.dll is based on the ledutil.c code in Mame. I just need someone to confirm it for me. Someone with some C experience obviously.

Take a look in ledutil.c (located in src\osd\windows) you will see in the function handle_copydata() the following two lines

Code: [Select]

// allocate memory
entry = malloc(sizeof(*entry));
string = malloc(strlen(data->string) + 1);
The "string" pointer is later assigned to entry->name and entry is added to a linked list. When Mame closes, this linked list is cleaned up in reset_id_to_outname_cache() like so

Code: [Select]

while (idmaplist != NULL)
{
id_map_entry *temp = idmaplist;
idmaplist = temp->next;
free(temp);
}
Above free(temp) there needs to be a free(entry->name) right?

[fatfingers]

Yes, if that is the only code-path for cleanup, then there is a memory leak there.  Note, however, if this is only ever executed by MAME shutdown code, then it's probably not a big deal as the OS will 'free' the memory when the application (MAME) exits.

[headkaze]

Yes, if that is the only code-path for cleanup, then there is a memory leak there.  Note, however, if this is only ever executed by MAME shutdown code, then it's probably not a big deal as the OS will 'free' the memory when the application (MAME) exits.

Yeah I thought so, and the thing is this code is designed to run all the time in applications like CPWizard and LEDBlinky so it is a problem because Mame can be launched hundreds of times while this code is still running, and each time the name string is allocating more memory that isn't free'd. So yes this is definately a problem. Thanks for confirming this, I was pretty sure it was a leak but was reluctant to second guess Aaron's code lol

[TheShanMan]

I just looked at ledutil.c as well. Definitely a leak. Good catch. Be sure to let Aaron or whoever know so it gets fixed in future releases.

[Cakemeister]

http://mamedev.org/submit.html

[headkaze]

I just want to clarify something before I report it as a bug, here is the definition of struct _id_map_entry

Code: [Select]

struct _id_map_entry
{
id_map_entry * next;
const char * name;
WPARAM id;
};
When I add in the free(entry->name) I get the following errors

src\osd\windows\ledutil.c(416) : error C2220: warning treated as error - no 'object' file generated
src\osd\windows\ledutil.c(416) : warning C4090: 'function' : different 'const' qualifiers
mingw32-make: *** [obj/windows/vmame/osd/windows/ledutil.o] Error 2

Should I be changing that to free((void*)entry->name);? Why is it a "const char *" instead of just a "char *"? Does it make a difference to free()?

[fatfingers]

The compiler is just being picky.  Your fix is fine.  The only way the fix might not be fine is if that memory wasn't truly dynamically allocated memory (i.e. it came from the stack somewhere), but from the example you have shown, that isn't the case.

[headkaze]

Okay thanks fatfingers, but I think the cast to remove the warning is better. free((void*)entry->name); it is.

EDIT: Okay I submitted a diff to mamedev

[TheShanMan]

Just to confirm what fatfingers said (I've examined the code while I think he's just gone by what you've posted here)... It's const just to make sure that after the string gets put into the structure, nothing else modifies it. The const part is all the compiler is upset about. It can free it as a char* or a void*, and there's absolutely nothing wrong with that. free() doesn't care what kind of pointer it is - only that it was allocated with malloc(). Your solution is the correct one.

And thanks for posting about this - I use the same code in a project I'm working on so I've benefited from your discovery.

[headkaze]

I've never had the need to use a const modifier for pointers before so that's what threw me. Mamedev specifically ask that there be no warnings for the code included in the diff's, so wanted to clarify it.

Appreciate the feedback (didn't manage to get any help at mameworld) 

[fatfingers]

I concur with TheShanMan's comments.

[headkaze]

This has been fixed in MAME 0.125u9

[brian_hoffman]

Yes it was.. I also see your name in the credits in (whats new)
Good job