Sign in to follow this  
sizeak

Can anyone help with a memory leak?

Recommended Posts

Hey, Im new here, just joined, mainly due to the issue I'm asking about. This isn't the cleantest code in the world, especially since its been messed up to hell trying to find this leak :D . I've got some code that works how it should fine, i'ts still in the very early stages and is only a small piece of what it will be once done. But anyway moving on to my problem, while it works, there seems to be a memory leak, a large one. Its the begginings of some server software, and it accepts connections, allocates a slot to them and creates a thread to handle there request/operation, whatever you wnat to call it. Atm all it can do is query a mysql database to see if they're username/password are valid. The problem is the more threads are made, the higher its memory usage gets, even though when a thread exits, it should, as far as i can tell be freeing all the memory it used. If no connections are being made, thus no threads being made, and just sitting idle, then the memory usage stops rising, but it doesnt drop at all. It just sits there. I've had it using over 600MB on linux before which is crazy. I just can't figure out where the leak is, I can't see anywhere in the code I've written that I'm leaking, its only 200 lines so far. There are 3 sources and 3 headers atm, I've included below: The header for main.cpp:
#ifndef MAIN_H
#define MAIN_H

#include <iostream>
#include <SDL/SDL.h>
#include <SDL/SDL_net.h>
#include <string>
#include <map>
#include <my_global.h>
#include <mysql.h>
using namespace std;

#ifdef main
#undef main
#endif

 unsigned long long int getCurrentTime();

#endif

Main.cpp:
#include "main.h"
#include "tcp_connect.h"
#include "auth.h"
#include <sys/timeb.h>

/************ Function Prtotypes *********************************************/
void clean(void);
static tcp_handler * tcpserver;

/*****************************************************************************/

int main(int argc, char *argv[])
{
	atexit(clean);	
    tcpserver = new tcp_handler();
    tcpserver->connect();

    return 0;
}

void clean(void)
{
	SDL_Quit();
    delete tcpserver;
}

 unsigned long long int getCurrentTime()
{
    struct timeb currentTime;
    unsigned long long int retval = 0ULL;
    
    ftime(&currentTime);
    
    retval = (unsigned long long int)((currentTime.time * 1000ULL) + currentTime.millitm);
    
    return retval;
}
The header for tcp_connect.cpp:
#ifndef TCP_CONNECT_H
#define TCP_CONNECT_H

#define SERVER_PORT 26000
#define PACKET_SIZE 4096
#define SLOTS 100


class tcp_handler
{
public:
    tcp_handler();
    ~tcp_handler(){};
    int connect(void);
    static int op_handler(void * sdata);	
private:
	int slotsFree();
    TCPsocket sd, csd, slotray[SLOTS]; /* Socket descriptor, Client socket descriptor */
	IPaddress ip, *remoteIP;
	int slot_num;	
	
	typedef struct socket_ptr
	{
		TCPsocket csd; // Keep this
		int slot_num;
	} sptr;	
};

#endif

tcp_connect.cpp:
#include "main.h"
#include  "tcp_connect.h"
#include "auth.h"

char slot_tracker[SLOTS];

tcp_handler::tcp_handler()
{
	memset(&slot_tracker, 0, SLOTS);	
}

 int tcp_handler::connect(void)
{	
	
    if (SDLNet_Init() < 0)
	{
		fprintf(stderr, "SDLNet_Init: %s\n", SDLNet_GetError());
		exit(EXIT_FAILURE);
	}

   	/* Resolving the host using NULL make network interface to listen */
	if (SDLNet_ResolveHost(&ip, NULL, SERVER_PORT) < 0)
	{
		fprintf(stderr, "SDLNet_ResolveHost: %s\n", SDLNet_GetError());
		exit(EXIT_FAILURE);
	}

	/* Open a connection with the IP provided (listen on the host's port) */
	if (!(sd = SDLNet_TCP_Open(&ip)))
	{
		fprintf(stderr, "SDLNet_TCP_Open: %s\n", SDLNet_GetError());
		exit(EXIT_FAILURE);
	}

	/* Wait for a connection, bind socket */

	while (1)
	{
		/* This checks sd for a pending connection.
            If there is one, accept that, and open a new socket for communicating */
		if ((csd = SDLNet_TCP_Accept(sd)))
		{
			slot_num = slotsFree();
			if(slot_num == -1)
			{
				SDLNet_TCP_Close(csd);
				cout << "Connect Refused - No Slots Free" << endl;
			}
			else
			{
				cout << "Connection Accepted - Slot: " << slot_num << endl;
				slotray[slot_num] = csd;
				slot_tracker[slot_num] = 1;

				/* Now we can communicate with the client using csd socket
				* sd will remain open waiting for other connections */
				/*thread creation here************************************************/
				/*create thread with op_handler function pasing socket csd, being the new connection*/
				sptr * stemp = new sptr;
				stemp->csd = slotray[slot_num];
				stemp->slot_num = slot_num;
				
				if(!SDL_CreateThread(op_handler, stemp))
				{
					cout << "Error creating thread" << endl;
				}
			}
		}		
    }	
}

/*************************************************************************************************/
int tcp_handler::op_handler(void * sdata)
{
	sptr * stemp = (sptr *)sdata;
    char packet[PACKET_SIZE];
    unsigned short op;
	cout << sdata << "\t" << stemp << endl;
	

    if(SDLNet_TCP_Recv(stemp->csd, &packet, PACKET_SIZE) < 0)
    {
        cout << "ERROR: No Packet Recieved. Exiting..." << endl;
        //exit(1);
    }
	else
	{
		op = packet[0];
		op <<= 8;
		op |= packet[1];
		
		switch (op)
		{
			case 1:
				auth_handler::auth_packet_handler(packet);
				break;
		}
	}
	
	SDLNet_TCP_Close(stemp->csd);
	slot_tracker[stemp->slot_num] = 0;
	delete stemp;
    return 0;
}

int tcp_handler::slotsFree()
{
	int i;
	
	for(i=0; i<SLOTS; i++)
	{
		if(!slot_tracker[i])
		{
			return i;
		}
	}
	
	return -1;
}

The header for auth.cpp:
#ifndef AUTH_H
#define AUTH_H

#define LOGIN_UNAME_LEN 32
#define LOGIN_PHASH_LEN 30

class auth_handler
{
public:
    auth_handler(){};
    ~auth_handler(){};
    static void auth_packet_handler(char *);
private:

};

#endif

auth.cpp:
#include "main.h"
#include "auth.h"

void auth_handler::auth_packet_handler(char * packet)
{
	MYSQL *conn = mysql_init(NULL);
    MYSQL_RES *Q_Res;
	MYSQL_ROW Q_Row;
	char SQL[256], username[32], passkey[30];
	int num_rows;
	std::string temp;
	
	memset(&username, 0, 32);
	memset(&passkey, 0, 30);
	mysql_init(conn);
	
	if(!mysql_real_connect(conn, "localhost", "root", "", "zero-sum", 0, NULL, 0))
	{
		cout << "ERROR: Unable to connect to MySQL" << endl;
        mysql_close(conn);
        exit(1);
	}
	
	strncpy(username, &packet[2], strlen(&packet[2]) + 1);
    strncpy(passkey, &packet[34], strlen(&packet[34]) + 1);
    sprintf(SQL, "SELECT * FROM `account status` WHERE exists (SELECT `userid` FROM `users` WHERE `key` = '%s' AND `username` = '%s')", passkey, username);	
	mysql_query(conn, SQL);	
	
	if (!(Q_Res = mysql_store_result(conn)))
    {
        printf("ERROR storing results for query string:\r\n%s\r\n", SQL);
        printf("Wrong username or password\r\n");
		//should kill thread here
    }
	else
	{   
		// Shouldn't happen, but better check
		num_rows = mysql_num_rows(Q_Res);
		
		if (num_rows == 0)
		{
			exit (-1);    // Invalid username or password
		}
		if (num_rows > 1)
		{
			exit (-2);    // Database setup error
		}
		
		// Everything's OK so far, now store the result in a row
		Q_Row = mysql_fetch_row(Q_Res);
		
		//this bit is just for a test
		//cout << "ID should be: " << Q_Row[0] << endl;
		temp = Q_Row[1];
		
		if(temp == "active")
		{
			//for now just print confirmation
			//cout << "Account Status: Active" << endl;
		}
		else if(Q_Row[1] == "inactive")
		{
			cout << "Acount Status: Inactive" << endl;
		}
		else if(Q_Row[1] == "banned")
		{
			cout << "Account Status: Banned" << endl;
		}
		else
		{
			cout << "Bollox, Error: " << Q_Row[1] << endl;
		}
	}		
	
	mysql_free_result(Q_Res);	
	mysql_close(conn);
}

I know some it is messy and probably newbish but I'm still learning, any help would be appreiciated :)

Share this post


Link to post
Share on other sites
I already ran it through valgrind a few times and from what i can tell it points to something in the thread libarys, i could be wrong though since i've never used it before.

Share this post


Link to post
Share on other sites
Quote:
Original post by sizeak
I already ran it through valgrind a few times and from what i can tell it points to something in the thread libarys, i could be wrong though since i've never used it before.


Valgrind can give lots of false positives if not properly configured. Not that I'm an expert, anyway.


You should isolate your problem, can't expect us to read all through (I have, however).

It seems good to me. Try commenting out the call to auth_handler::auth_packet_handler, and see if it keeps leaking.

Share this post


Link to post
Share on other sites
i just took a quick look...

this looks kinda suspicious...


sptr * stemp = new sptr;
stemp->csd = slotray[slot_num];
stemp->slot_num = slot_num;

if(!SDL_CreateThread(op_handler, stemp))
{
cout << "Error creating thread" << endl;
}




u never delete stemp...

have u considered using smart pointers? that would probably reduce memory leakage problems

EDIT: i second Evil Steve's "Free Memory Leak Checker (C++)"

Share this post


Link to post
Share on other sites
Quote:
Original post by Dragon_Strike
this looks kinda suspicious...



[...]
SDLNet_TCP_Close(stemp->csd);
slot_tracker[stemp->slot_num] = 0;
delete stemp;
return 0;
}


He deletes it just before returning from the thread callback.

Share this post


Link to post
Share on other sites
Quote:
Original post by ravengangrel
Quote:
Original post by Dragon_Strike
this looks kinda suspicious...



[...]
SDLNet_TCP_Close(stemp->csd);
slot_tracker[stemp->slot_num] = 0;
delete stemp;
return 0;
}


He deletes it just before returning from the thread callback.


I'm glad someone is on their toes ;)

I'll give your suggestions a go and let you know how it goes

Share this post


Link to post
Share on other sites
The code you provided contains exactly two instances of memory allocations from the free store (one directly in main(), one in tcp_handler::connect()) ... and you're cleaning up properly after both of these (but you would not if that exit(1) in tcp_handler::op_handler() wasn't commented out).
If your program contains any memory leaks, it's not in the code you showed us.

That said, I second Dragon_Strike's suggestion. Use smart pointers (e.g. boost.smart ptr or even just std::auto_ptr) and other RAII guards that automatically clean up after themselves. Get in the habit of programming in an exception-safe manner (which RAII is the best tool for), and resource leaks will be a thing of the past.

Share this post


Link to post
Share on other sites
Quote:
Original post by Red Ant
The code you provided contains exactly two instances of memory allocations from the free store (one directly in main(), one in tcp_handler::connect()) ... and you're cleaning up properly after both of these (but you would not if that exit(1) in tcp_handler::op_handler() wasn't commented out).
If your program contains any memory leaks, it's not in the code you showed us.

That said, I second Dragon_Strike's suggestion. Use smart pointers (e.g. boost.smart ptr or even just std::auto_ptr) and other RAII guards that automatically clean up after themselves. Get in the habit of programming in an exception-safe manner (which RAII is the best tool for), and resource leaks will be a thing of the past.


Thans for the reply, I'll look into what you mentioned. But like you say, my code isnt leaking so there must be a problem with either the mysql api or with SDL. After haiving many issues with boost asio, I decided that was pure evil and needed purging

Edit: RAII sounds like a good stratergy, though in the code as it is now, I ended up writing in a C procedural style, making the classes redundant, I should really re-write it before its gets to messy...

Share this post


Link to post
Share on other sites
Quote:
Original post by Red Ant
The code you provided contains exactly two instances of memory allocations from the free store (one directly in main(), one in tcp_handler::connect()) ... and you're cleaning up properly after both of these (but you would not if that exit(1) in tcp_handler::op_handler() wasn't commented out).
If your program contains any memory leaks, it's not in the code you showed us.


Your post is misleading. Although it's true, it can make the OP think that his code is correct, and that other library is the culprit and I don't think so. He is cleaning the resources he directly allocates but perhaps some call to SDL or mysql needs the caller to release something which he isn't freeing.

Share this post


Link to post
Share on other sites
Quote:
Original post by ravengangrel
Quote:
Original post by Red Ant
The code you provided contains exactly two instances of memory allocations from the free store (one directly in main(), one in tcp_handler::connect()) ... and you're cleaning up properly after both of these (but you would not if that exit(1) in tcp_handler::op_handler() wasn't commented out).
If your program contains any memory leaks, it's not in the code you showed us.


Your post is misleading. Although it's true, it can make the OP think that his code is correct, and that other library is the culprit and I don't think so. He is cleaning the resources he directly allocates but perhaps some call to SDL or mysql needs the caller to release something which he isn't freeing.


I think you may be right, after doing some reading i found this:

"SDL 1.2.7) Even after the procedure started in the thread returns, there still exist some resources allocated to the thread. To free these resources, use SDL_WaitThread to wait for the thread to finish and obtain the status code of the thread. If not done so, SDL_CreateThread will hang after about 1010 successfully created threads (tested on GNU/Linux). This is consistent with POSIX threads behavior where unless threads are explicitely detached using pthread_detach() or created using a pthread_attr initialized using pthread_attr_setdetachstate() passed at pthread_create(), they must be waited for using pthread_join() for their resources to be released. The SDL threads abstraction library does not provide the functions to detach a thread to launch a thread in detached mode."

Taken from: http://www.libsdl.org/cgi/docwiki.cgi/SDL_CreateThread#

Though as far as I can tell, SDL_WaitThread would block, which is bad for me, since it would break my loop. I may have to switch to detached pthreads

Share this post


Link to post
Share on other sites
Right, Ive fixed one leak to do with waiting for SDL threads since they're not detatched. Though doing it this way slows my code and doesnt really fit with the model I'm aiming for, so i may have to switch to detatched pthreads and write it for linux only.

Commenting out the call to the auth handler, like suggested, solves the other leak, which indicates to me that it much be a mysql issues of some kind. Thats the next thing I'll have to look inot. Thanks for the help so far :D

EDIT: I've just fixed the remaining issue, the problem was, I wasn't calling mysql_library_end(); which thus wasnt doing its cleanup. Now I just have to either work around SDL threads or switch to detatched pthreads.

Thanks for the help though, It was helpfull just to have people to bounce ideas off of, thanks

[Edited by - sizeak on June 4, 2009 12:36:02 PM]

Share this post


Link to post
Share on other sites
I thought about the thread, but I had never used them, so I took a look around the docs but couldn't find anything.

Same problem with mysql, Doesn't Q_Res need to be released?

Edit:

About the ThreadWait issue, perhaps you could manage a list of finished threads and when one goes into the list you call WaitThread.

Or just wrap it into a class so it gets auto released.

Also, you could use wxWidgets which has a wxThread class which can be created as "Fire and forget". But perhaps it's overkill to link against wxWidgets just for the threads.

Share this post


Link to post
Share on other sites
Quote:
Original post by ravengangrel
I thought about the thread, but I had never used them, so I took a look around the docs but couldn't find anything.

Same problem with mysql, Doesn't Q_Res need to be released?

Edit:

About the ThreadWait issue, perhaps you could manage a list of finished threads and when one goes into the list you call WaitThread.

Or just wrap it into a class so it gets auto released.

Also, you could use wxWidgets which has a wxThread class which can be created as "Fire and forget". But perhaps it's overkill to link against wxWidgets just for the threads.


I free Q_Res, just not Q_Row, but ive solved all the leaks now. I think the reason it doesnt offer detatched is that windows only has joined threads, I'm pretty sure thats what I read. Someone correct me if I'm wrong :)

Share this post


Link to post
Share on other sites
Quote:
Original post by sizeak
I free Q_Res, just not Q_Row, but ive solved all the leaks now. I think the reason it doesnt offer detatched is that windows only has joined threads, I'm pretty sure thats what I read. Someone correct me if I'm wrong :)


I don't think so, you can do

HANDLE hThread = CreateThread([...]);
CloseHandle (hThread);

And it will free its resources when its done (You still must call CloseHandle, but it doesn't wait for anything)

Share this post


Link to post
Share on other sites
Quote:
Original post by ravengangrel
Your post is misleading. Although it's true, it can make the OP think that his code is correct, and that other library is the culprit and I don't think so. He is cleaning the resources he directly allocates but perhaps some call to SDL or mysql needs the caller to release something which he isn't freeing.



Well, you obviously have a point there. *hangs head in shame*

Share this post


Link to post
Share on other sites
Quote:
Original post by Red Ant
Quote:
Original post by ravengangrel
Your post is misleading. Although it's true, it can make the OP think that his code is correct, and that other library is the culprit and I don't think so. He is cleaning the resources he directly allocates but perhaps some call to SDL or mysql needs the caller to release something which he isn't freeing.



Well, you obviously have a point there. *hangs head in shame*


Lol, Thanks for trying anyway

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this