Thread: Memory Leak

In my app, each time the listView refreshes, the memory increases. When I debugged it, the dwProcesses function below kept repeating itself. I think this function is chewing up memory, but don't know how to stop it. I tried using calloc, so I could free it but free does not seem to do anything. I mainly want to have all dwProcesses elements set to 0 when the function is not executing, but I can't figure out how this is acheived. Also this function is called each time the ListView refreshes. Say so If you want me to post the whole program.

Code:

int getProcesses() {

	HMODULE hModule;
	char szProcessName[MAX_PATH] = {0};
    DWORD dwProcesses[1024], cbNeeded, cProcesses;
	*dwProcesses = calloc(1024, sizeof (DWORD));
	if (dwProcesses == NULL) return 0;
	unsigned int i;
	char PIDbuf[10];
	char buf[50];

	if (!EnumProcesses(dwProcesses, sizeof(dwProcesses), &cbNeeded))
		return -1;
	cProcesses = cbNeeded / sizeof(DWORD);
	for (i = 0; i < cProcesses; i++)
		if(dwProcesses[i] != 0)
		{
			HANDLE hProcess = OpenProcess(PROCESS_QUERY_INFORMATION |
				PROCESS_VM_READ, FALSE, dwProcesses[i]);
			if (NULL != hProcess)
			{
				strcpy(szProcessName, "System");
				if (EnumProcessModules(hProcess, &hModule, sizeof(hModule),
					&cbNeeded))
				{
					GetModuleBaseName(hProcess, hModule, szProcessName,
						sizeof(szProcessName)/sizeof(CHAR));
				}
			sprintf(PIDbuf, "%d", dwProcesses[i]);
			InsertRow(GetDlgItem(hwndMain, ID_LISTVIEW), 6, szProcessName,PIDbuf,
			GetProcessMemoryWSS(hProcess, buf), GetProcessMemoryPWSS(hProcess, buf),
		    GetProcessPriority(hProcess), GetUserInfo(dwProcesses[i], hProcess));
			}
			free(dwProcesses[i-1]);
			CloseHandle(hProcess);
		}
	return cProcesses;
}
--dwProceses Trace:
dwProcesses0,4,908,980,1004,1048,1060,1228,1308,1888,280,588,944,1448,1764...
dwProcesses0,4,908,980,1004,1048,1060,1228,1308,1888,280,588,944,1448,1764...
dwProcesses0,4,908,980,1004,1048,1060,1228,1308,1888,280,588,944,1448,1764...
--keeps printing out the same line

Shouldnt "DWORD dwProcesses[1024]" be "DWORD* dwProcesses"?

It should look something like this:

Code:

int getProcesses() {

	HMODULE hModule;
	char szProcessName[MAX_PATH] = {0};
        DWORD* dwProcesses, cbNeeded, cProcesses;
	dwProcesses = calloc(1024, sizeof (DWORD));
	unsigned int i;
	char PIDbuf[10];
	char buf[50];

	if (!EnumProcesses(dwProcesses, sizeof(*dwProcesses) * 1024, &cbNeeded))
		return -1;
	cProcesses = cbNeeded / sizeof(DWORD);
	for (i = 0; i < cProcesses; i++)
        {
		if(dwProcesses[i] != 0)
		{
			HANDLE hProcess = OpenProcess(PROCESS_QUERY_INFORMATION |
				PROCESS_VM_READ, FALSE, dwProcesses[i]);
			if (NULL != hProcess)
			{
				strcpy(szProcessName, "System");
				if (EnumProcessModules(hProcess, &hModule, sizeof(hModule),
					&cbNeeded))
				{
					GetModuleBaseName(hProcess, hModule, szProcessName,
						sizeof(szProcessName)/sizeof(CHAR));
				}
			        sprintf(PIDbuf, "&#37;d", dwProcesses[i]);
			        InsertRow(GetDlgItem(hwndMain, ID_LISTVIEW), 6, szProcessName,PIDbuf,
			        GetProcessMemoryWSS(hProcess, buf), GetProcessMemoryPWSS(hProcess, buf),
		                GetProcessPriority(hProcess), GetUserInfo(dwProcesses[i], hProcess));
			}
			CloseHandle(hProcess);
		}
        }
        free(dwProcesses);
	return cProcesses;
}

EDIT: Actually looking at your code again, you could of had it exactly how you had it but just without the dynamic allocation and free

This might be a better solution

Code:

int getProcesses() {

	HMODULE hModule;
	char szProcessName[MAX_PATH] = {0};
        DWORD dwProcesses[1024], cbNeeded, cProcesses;
	unsigned int i;
	char PIDbuf[10];
	char buf[50];

	if (!EnumProcesses(dwProcesses, sizeof(dwProcesses), &cbNeeded))
		return -1;
	cProcesses = cbNeeded / sizeof(DWORD);
	for (i = 0; i < cProcesses; i++)
        {
		if(dwProcesses[i] != 0)
		{
			HANDLE hProcess = OpenProcess(PROCESS_QUERY_INFORMATION |
				PROCESS_VM_READ, FALSE, dwProcesses[i]);
			if (NULL != hProcess)
			{
				strcpy(szProcessName, "System");
				if (EnumProcessModules(hProcess, &hModule, sizeof(hModule),
					&cbNeeded))
				{
					GetModuleBaseName(hProcess, hModule, szProcessName,
						sizeof(szProcessName)/sizeof(CHAR));
				}
			        sprintf(PIDbuf, "%d", dwProcesses[i]);
			        InsertRow(GetDlgItem(hwndMain, ID_LISTVIEW), 6, szProcessName,PIDbuf,
			        GetProcessMemoryWSS(hProcess, buf), GetProcessMemoryPWSS(hProcess, buf),
		                GetProcessPriority(hProcess), GetUserInfo(dwProcesses[i], hProcess));
			}
			CloseHandle(hProcess);
		}
        }
	return cProcesses;
}

Changed. I still don't get a different result (except the line it prints out gives the value in hex). When Dev-C++ runs through each line in the for loop, it generates another dwProcesses and does not clear it when the free is executed.

Code:

    DWORD dwProcesses[1024], cbNeeded, cProcesses;
    *dwProcesses = calloc(1024, sizeof (DWORD));
    ......
    free(dwProcesses[i-1]);

You do not have to reallocate local arrays! 39ster's second code should fix your problems unless you messed up something in other functions too.

Code:

    DWORD dwProcesses[1024], cbNeeded, cProcesses;
	*dwProcesses = calloc(1024, sizeof (DWORD));

I'm incredibly surprised that even compiles. You cannot dereference an array!
And furthermore, calloc returns a pointer, so you mustn't dereference a pointer and then assign it - you must assign it TO the pointer!

With that solution, what happens to dwProcesses after return cProcesses is executed? It just stays there, wasting memory. Also the edit was what I originally had, with the problem. I have attached the archived project, so the memory leak will become obvious. After compiling it, goto view>Update Speed>High. To make the leak visible (it isn't an error, task manager does the same thing). Normally the memory usage should not act as a counter. Comments on the program would be appreciated.
I think the best option is to perform something similar to closeHandle with dwProcesses (clear the value when it is not being used, I know they are completely different data types and doing this directly would not work). I was thinking of not using dynamic memory allocation, but I don't know another way to clear the value of a variable without either setting it to 0, NULL or using free (and this is an array).

As you know, variables allocated on the stack are automatically cleaned after the functions returns.
Dynamic memory may or may not get freed when you call free on it (OS dependant), but you can't use it.
The OS managed dynamic memory at will and you're at its mercy when you use it.

The short version of Elysia's post: Local variables don't leak.

Thanks for clearing that up. Sorry, I was wrong, GetProcesses is not creating the memory leak, it is GetUserInfo thats causing the problem. I have tried to make sure all the handles are closed and variables that are dynamically allocated are freed, also it isn't the function being called before it can finish. This raises the question, what is causing it? As 2/3 times the function is called, it consumes an extra 4k of memory. Taskman doesn't do that.
Edit: I fixed up some warnings and the memory usage does not go up as fast

What made you think it was leaking? At the very least if you overwrote your allocation you would get an exception something like this:

Memory around variable 'x' has been corrupted.

Meaning that you wrote past the end of an array on the stack.


You should use the debug CRT to find your memory leaks or some 3rd party program. Some 3rd party leak detectors give off so many false positives as to be useless but the CRT usually works just fine for me.
Check in your compiler help about how to use the debug CRT. There is a ton of functionality that is not turned on by default. Of course I'm talking about MSVC so if you are not using that then this may not help you.

Originally Posted by Bubba

What made you think it was leaking?

Well considering the program is a task manager, where the leak was occurring and how to replicate it, I was led to believe that that variable was leaking. Also Dev-C++'s debugger didn't help. I have realised that part of the problem was due to some warnings about parameters being passed as the wrong type. The application's memory usage now does perform as expected. Anyway, lcc-Win32 can take part of the blame for not giving those warnings and returning corrupt data from the getuserinfo function (fixed now). I don't have MSVC so I guess the debug CRT is not an option. I will still leave the source code up (for the time being), in case anyone wants to look at it/make suggestions. Thanks anyway.

Originally Posted by P4R4N01D

I don't have MSVC so I guess the debug CRT is not an option. I will still leave the source code up (for the time being), in case anyone wants to look at it/make suggestions. Thanks anyway.

You could just download the Express version. It contains full debugger capabilities.

Originally Posted by Bubba

A simple review of C++ shows that you absolutely cannot leak the memory you thought you were leaking.

I am using C, but the compiler should atleast give a warning when a possible memory leak could occur. The program is better now, it was due to parameters being passed to functions (GetUserNameA etc.) as the wrong type. The compiler gave these warnings and i fixed them up (eventually). If I am wrong and it starts leaking I will get the debug CRT, hopefully that will help. In the future I will hopefully test the app. and fix eany bugs that it may have. Dev-C++ is better than lcc-Win32. Thanks to you all for helping and directing be to the blantently obvious.