Fixed memory leaks

Makefile

@@ -7,7 +7,7 @@ man8 := $(DESTDIR)/usr/share/man/man8/
 
 all: nethogs
 
-CFLAGS=-g -Wall -pg
+CFLAGS=-g -Wall
 #CFLAGS=-O2
 OBJS=structs.o packet.o connection.o process.o refresh.o decpcap.o cui.o inode2prog.o
 GCC=g++

connection.cpp

@@ -31,6 +31,7 @@ void PackList::add (Packet * p)
 		return;
 	}
 
+	/* store copy of packet, so that original may be freed */
 	content = new PackListNode(new Packet (*p), content);
 }
 
@@ -60,6 +61,7 @@ u_int32_t PackList::sumanddel (timeval t)
 	return retval;
 }
 
+/* packet may be deleted by caller */
 Connection::Connection (Packet * packet)
 {
 	if (ROBUST)

connection.h

@@ -44,6 +44,7 @@ public:
 	/* sums up the total bytes used and removes 'old' packets */
 	u_int32_t sumanddel (timeval t);
 
+	/* calling code may delete packet */
 	void add (Packet * p);
 private:
 	PackListNode * content;
@@ -55,6 +56,7 @@ public:
 	/* constructs a connection, makes a copy of
 	 * the packet as 'refpacket', and adds the
 	 * packet to the packlist */
+        /* packet may be deleted by caller */
 	Connection (Packet * packet);
 
 	~Connection();
@@ -83,6 +85,7 @@ private:
 };
 
 /* Find the connection this packet belongs to */
+/* (the calling code may free the packet afterwards) */
 Connection * findConnection (Packet * packet);
 
 #endif

cui.cpp

@@ -17,8 +17,13 @@ extern Process * unknownproc;
 class Line 
 {
 public:
-	Line (const char * name, double n_sent_kbps, double n_recv_kbps, int pid, uid_t uid, const char * n_devicename)
+	Line (const char * name, double n_sent_kbps, double n_recv_kbps, pid_t pid, uid_t uid, const char * n_devicename)
 	{
+		if (ROBUST) 
+		{
+			assert (uid >= 0);
+			assert (pid >= 0);
+		}
 		m_name = name; 
 		sent_kbps = n_sent_kbps; 
 		recv_kbps = n_recv_kbps;
@@ -39,22 +44,23 @@ public:
 private:
 	const char * m_name;
 	const char * devicename;
-	int m_pid;
-	int m_uid;
+	pid_t m_pid;
+	uid_t m_uid;
 };
 
-char * uid2username (int uid)
+char * uid2username (uid_t uid)
 {
 	struct passwd * pwd = NULL; 
 	/* getpwuid() allocates space for this itself, 
 	 * which we shouldn't free */
 	pwd = getpwuid(uid);
 
-	if (ROBUST)
-		assert (pwd != NULL);
-
 	if (pwd == NULL)
 	{
+		if (ROBUST)
+		{
+			assert(false);
+		}
 		return strdup ("unlisted");
 	} else {
 		return strdup(pwd->pw_name);
@@ -169,7 +175,7 @@ void do_refresh()
 	double sent_global = 0;
 	double recv_global = 0;
 
-	if (DEBUG)
+	if (ROBUST)
 	{
 		// initialise to null pointers
 		for (int i = 0; i < nproc; i++)
@@ -185,6 +191,7 @@ void do_refresh()
 		{
 			assert (curproc != NULL);
 			assert (curproc->getVal() != NULL);
+			assert (nproc == processes->size());
 		}
 		/* do not remove the unknown process */
 		if ((curproc->getVal()->getLastPacket() + PROCESSTIMEOUT <= curtime.tv_sec) && (curproc->getVal() != unknownproc))
@@ -207,48 +214,62 @@ void do_refresh()
 			nproc--;
 			//continue;
 		}
-		else{
-
-		u_int32_t sum_sent = 0, 
-			  sum_recv = 0;
-
-		/* walk though all this process's connections, and sum them
-		 * up */
-		ConnList * curconn = curproc->getVal()->connections;
-		ConnList * previous = NULL;
-		while (curconn != NULL)
+		else
 		{
-			if (curconn->getVal()->getLastPacket() <= curtime.tv_sec - CONNTIMEOUT)
+
+			u_int32_t sum_sent = 0, 
+			  	sum_recv = 0;
+
+			/* walk though all this process's connections, and sum them
+			 * up */
+			ConnList * curconn = curproc->getVal()->connections;
+			ConnList * previous = NULL;
+			while (curconn != NULL)
 			{
-				/* stalled connection, remove. */
-				ConnList * todelete = curconn;
-				Connection * conn_todelete = curconn->getVal();
-				curconn = curconn->getNext();
-				if (todelete == curproc->getVal()->connections)
-					curproc->getVal()->connections = curconn;
-				if (previous != NULL)
-					previous->setNext(curconn);
-				delete (todelete);
-				delete (conn_todelete);
+				if (curconn->getVal()->getLastPacket() <= curtime.tv_sec - CONNTIMEOUT)
+				{
+					/* stalled connection, remove. */
+					ConnList * todelete = curconn;
+					Connection * conn_todelete = curconn->getVal();
+					curconn = curconn->getNext();
+					if (todelete == curproc->getVal()->connections)
+						curproc->getVal()->connections = curconn;
+					if (previous != NULL)
+						previous->setNext(curconn);
+					delete (todelete);
+					delete (conn_todelete);
+				} 
+				else 
+				{
+					u_int32_t sent = 0, recv = 0;
+					curconn->getVal()->sumanddel(curtime, &sent, &recv);
+					sum_sent += sent;
+					sum_recv += recv;
+					previous = curconn;
+					curconn = curconn->getNext();
+				}
 			}
-			else 
+			uid_t uid = curproc->getVal()->getUid();
+			if (ROBUST)
 			{
-				u_int32_t sent = 0, recv = 0;
-				curconn->getVal()->sumanddel(curtime, &sent, &recv);
-				sum_sent += sent;
-				sum_recv += recv;
-				previous = curconn;
-				curconn = curconn->getNext();
+				assert (getpwuid(uid) != NULL);
+				assert (curproc->getVal()->pid >= 0);
+				assert (n < nproc);
+			}
+			lines[n] = new Line (curproc->getVal()->name, tokbps(sum_sent), tokbps(sum_recv), 
+					curproc->getVal()->pid, uid, curproc->getVal()->devicename);
+			previousproc = curproc;
+			curproc = curproc->next;
+			n++;
+			if (ROBUST)
+			{
+				assert (nproc == processes->size());
+				if (curproc == NULL)
+					assert (n-1 < nproc);
+				else
+					assert (n < nproc);
+
 			}
-		}
-		uid_t uid = curproc->getVal()->getUid();
-		if (ROBUST)
-			assert (uid >= 0);
-		lines[n] = new Line (curproc->getVal()->name, tokbps(sum_sent), tokbps(sum_recv), 
-				curproc->getVal()->pid, uid, curproc->getVal()->devicename);
-		previousproc = curproc;
-		curproc = curproc->next;
-		n++;
 		}
 	}
 

inode2prog.cpp

@@ -55,6 +55,7 @@ char * getprogname (char * pid) {
 	int fd = open(filename, O_RDONLY);
 	if (fd < 0) {
 		fprintf (stderr, "Error opening %s: %s\n", filename, strerror(errno));
+		free (filename);
 		exit(3);
 		return NULL;
 	}
@@ -63,6 +64,7 @@ char * getprogname (char * pid) {
 		cout << "Error closing file: " << strerror(errno) << endl;
 		exit(34);
 	}
+	free (filename);
 	if (length < bufsize - 1)
 		buffer[length]='\0';
 
@@ -76,6 +78,13 @@ char * getprogname (char * pid) {
 	return strdup(retval);
 }
 
+void setnode (unsigned long inode, prg_node * newnode)
+{
+	if (inodeproc[inode] != NULL)
+		free (inodeproc[inode]);
+	inodeproc[inode] = newnode;
+}
+
 void get_info_by_linkname (char * pid, char * linkname) {
 	if (strncmp(linkname, "socket:[", 8) == 0) {
 		char * ptr = linkname + 8;
@@ -90,7 +99,7 @@ void get_info_by_linkname (char * pid, char * linkname) {
 		// TODO progname could be more memory-efficient
 		strncpy (newnode->name, progname, PROGNAME_WIDTH);
 		free (progname);
-		inodeproc[inode] = newnode;
+		setnode (inode, newnode);
 	} else {
 		//std::cout << "Linkname looked like: " << linkname << endl;
 	}
@@ -128,6 +137,11 @@ void get_info_for_pid(char * pid) {
 		int linklen = 80;
 		char linkname [linklen];
 		int usedlen = readlink(fromname, linkname, linklen-1);
+		if (usedlen == -1)
+		{
+			free (fromname);
+			continue;
+		}
 		if (ROBUST)
 			assert (usedlen < linklen);
 		linkname[usedlen] = '\0';

nethogs.cpp

@@ -107,12 +107,12 @@ int process_tcp (u_char * userdata, const dp_header * header, const u_char * m_p
 	{
 		/* add packet to the connection */
 		connection->add(packet);
-		delete packet;
 	} else {
 		/* else: unknown connection, create new */
 		connection = new Connection (packet);
 		getProcess(connection, currentdevice);
 	}
+	delete packet;
 
 	if (needrefresh)
 	{

process.cpp

@@ -28,7 +28,7 @@ extern local_addr * local_addrs;
  * key contains source ip, source port, destination ip, destination 
  * port in format: '1.2.3.4:5-1.2.3.4:5'
  */
-std::map <std::string, unsigned long *> conninode;
+std::map <std::string, unsigned long> conninode;
 
 /*
  * Initialise the global process-list with `the' unknown process
@@ -82,18 +82,19 @@ void addtoconninode (char * buffer)
     	struct in6_addr in6_local;
     	struct in6_addr in6_remote;
 
-	// the following leaks some memory.
-	unsigned long * inode = (unsigned long *) malloc (sizeof(unsigned long));
+	// this leaked memory
+	//unsigned long * inode = (unsigned long *) malloc (sizeof(unsigned long));
+	unsigned long inode;
 
 	int matches = sscanf(buffer, "%*d: %64[0-9A-Fa-f]:%X %64[0-9A-Fa-f]:%X %*X %*X:%*X %*X:%*X %*X %*d %*d %ld %*512s\n",
-		local_addr, &local_port, rem_addr, &rem_port, inode);
+		local_addr, &local_port, rem_addr, &rem_port, &inode);
 
 	if (matches != 5) {
 		fprintf(stderr,"Unexpected buffer: '%s'\n",buffer);
 		exit(0);
 	}
 	
-	if (*inode == 0) {
+	if (inode == 0) {
 		/* connection is in TIME_WAIT state. We rely on 
 		 * the old data still in the table. */
 		return;
@@ -229,14 +230,14 @@ void reviewUnknown ()
 	ConnList * previous_conn = NULL;
 
 	while (curr_conn != NULL) {
-		unsigned long * inode = conninode[curr_conn->getVal()->refpacket->gethashstring()];
-		if (inode != NULL)
+		unsigned long inode = conninode[curr_conn->getVal()->refpacket->gethashstring()];
+		if (inode != 0)
 		{
-			Process * proc = findProcess (*inode);
+			Process * proc = findProcess (inode);
 			if (proc != unknownproc && proc != NULL)
 			{
 				if (DEBUG)
-					std::cout << "ITP: WARNING: Previously unknown inode " << *inode << " now got process...??\n";
+					std::cout << "ITP: WARNING: Previously unknown inode " << inode << " now got process...??\n";
 				/* Yay! - but how could this happen? */
 				//assert(false);
 				if (previous_conn != NULL)
@@ -315,7 +316,7 @@ Process * getProcess (unsigned long inode, char * devicename)
 	if (proc != NULL)
 		return proc;
 
-	Process * newproc = new Process (inode, strdup(devicename));
+	Process * newproc = new Process (inode, devicename);
 	newproc->name = strdup(node->name);
 	newproc->pid = node->pid;
 
@@ -325,6 +326,7 @@ Process * getProcess (unsigned long inode, char * devicename)
 	stat(procdir, &stats);
 	newproc->setUid(stats.st_uid);
 
+	assert (getpwuid(stats.st_uid) != NULL);
 	processes = new ProcList (newproc, processes);
 	return newproc;
 }
@@ -338,9 +340,9 @@ Process * getProcess (unsigned long inode, char * devicename)
  */
 Process * getProcess (Connection * connection, char * devicename)
 {
-	unsigned long * inode = conninode[connection->refpacket->gethashstring()];
+	unsigned long inode = conninode[connection->refpacket->gethashstring()];
 
-	if (inode == NULL)
+	if (inode == 0)
 	{
 		// no? refresh and check conn/inode table
 #if DEBUG
@@ -348,7 +350,7 @@ Process * getProcess (Connection * connection, char * devicename)
 #endif
 		refreshconninode();
 		inode = conninode[connection->refpacket->gethashstring()];
-		if (inode == NULL)
+		if (inode == 0)
 		{
 			/* HACK: the following is a hack for cases where the 
 			 * 'local' addresses aren't properly recognised, as is 
@@ -360,7 +362,7 @@ Process * getProcess (Connection * connection, char * devicename)
 			Packet * reversepacket = connection->refpacket->newInverted();
 			inode = conninode[reversepacket->gethashstring()];
 
-			if (inode == NULL)
+			if (inode == 0)
 			{
 				delete reversepacket;
 				if (DEBUG)
@@ -374,7 +376,7 @@ Process * getProcess (Connection * connection, char * devicename)
 		}
 	}
 
-	Process * proc = getProcess(*inode, devicename);
+	Process * proc = getProcess(inode, devicename);
 	proc->connections = new ConnList (connection, proc->connections);
 	return proc;
 }

process.h

@@ -66,16 +66,18 @@ public:
 			assert (uid >= 0);
 		}
 	}
-	/* TODO free m_name and m_devicename again in constructor */
+	
 	~Process ()
 	{
+		free (name);
+		free (devicename);
 		if (DEBUG)
 			std::cout << "PROC: Process deleted at " << this << std::endl;
 	}
 	int getLastPacket ();
 
-	const char * name;
-	const char * devicename;
+	char * name;
+	char * devicename;
 	int pid;
 
 	unsigned long inode;