Can anyone help with a memory leak?

Started by
15 comments, last by sizeak 14 years, 10 months ago
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)
		{
			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 :)
Advertisement
I don't have time to sift through that code, but perhaps this will help you
Free Memory Leak Checker (C++)
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.
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.
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++)"
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.
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
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.
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...
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.

This topic is closed to new replies.

Advertisement