hunting memory management bug

DESIGN

@@ -28,3 +28,5 @@ connection.cpp:
   connections. 'ConnList' list containting all currently known connections.
   A connection removes itself from the global 'connections' list in its 
   destructor.
+
+  processes. 'ProcList *' containing all processes.

Makefile

@@ -7,7 +7,7 @@ man8 := $(DESTDIR)/usr/share/man/man8/
 
 all: nethogs
 
-CFLAGS=-g
+CFLAGS=-g -Wall
 OBJS=structs.o packet.o connection.o process.o hashtbl.o refresh.o decpcap.o
 GCC=g++
 .PHONY: tgz

hashtbl.cpp

@@ -68,7 +68,7 @@ void HashTable::add(char * key, void * content)
 {
 	char * localkey = strdup(key);
 	unsigned int hkey = HashString (localkey);
-	//std::cout << "(STILL)Adding node: " << localkey << " key " << hkey << endl;
+	//std::cout << "LOC: Adding node: " << localkey << " key " << hkey << endl;
 	table[hkey] = newHashNode(localkey, content, table[hkey]);
 }
 

nethogs.cpp

@@ -112,7 +112,7 @@ int process_tcp (u_char * userdata, const dp_header * header, const u_char * m_p
 	} else {
 		/* else: unknown connection, create new */
 		connection = new Connection (packet);
-		Process * process = getProcess(connection, currentdevice);
+		getProcess(connection, currentdevice);
 	}
 
 	if (needrefresh)

nethogs.h

@@ -30,7 +30,7 @@
 #define NEEDROOT 1
 #endif
 
-#define DEBUG 0
+#define DEBUG 1
 
 // 2 times: 32 characters, 7 ':''s, a ':12345'.
 // 1 '-'

packet.cpp

@@ -216,6 +216,7 @@ bool Packet::Outgoing () {
 		  	return false;
 		}
 	}
+	return false;
 }
 
 /* returns the packet in '1.2.3.4:5-1.2.3.4:5'-form, for use in the 'conninode' table */
@@ -225,8 +226,6 @@ char * Packet::gethashstring ()
 {
 	if (hashstring != NULL)
 	{
-		if (DEBUG)
-			std::cout << "Returning cached hash string: " << hashstring << std::endl;
 		return hashstring;
 	}
 
@@ -248,8 +247,8 @@ char * Packet::gethashstring ()
 	}
 	free (local_string);
 	free (remote_string);
-	if (DEBUG)
+	//if (DEBUG)
-		std::cout << "Returning newly created hash string: " << hashstring << std::endl;
+	//	std::cout << "Returning newly created hash string: " << hashstring << std::endl;
 	return hashstring;
 }
 

process.cpp

@@ -8,12 +8,12 @@
 #include <stdlib.h>
 #include <pwd.h>
 #include <sys/types.h>
+#include <map>
 
 #include "process.h"
 #include "hashtbl.h"
 #include "nethogs.h"
 #include "inodeproc.cpp"
-//#include "inet6.c"
 
 extern timeval curtime;
 extern std::string * caption;
@@ -40,11 +40,30 @@ private:
  * key contains source ip, source port, destination ip, destination 
  * port in format: '1.2.3.4:5-1.2.3.4:5'
  */
-HashTable * conninode = new HashTable (256);
+//HashTable * conninode = new HashTable (256);
+std::map <std::string, unsigned long *> conninode;
 
 Process * unknownproc = new Process (0, "", "unknown");
 ProcList * processes = new ProcList (unknownproc, NULL);
 
+int Process::getLastPacket()
+{
+	int lastpacket=0;
+	ConnList * curconn=connections;
+	while (curconn != NULL)
+	{
+		if (DEBUG)
+		{
+			assert (curconn != NULL);
+			assert (curconn->getVal() != NULL);
+		}
+		if (curconn->getVal()->getLastPacket() > lastpacket)
+			lastpacket = curconn->getVal()->getLastPacket();
+		curconn = curconn->getNext();
+	}
+	return lastpacket;
+}
+
 /*
  * parses a /proc/net/tcp-line of the form:
  *     sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
@@ -55,7 +74,6 @@ ProcList * processes = new ProcList (unknownproc, NULL);
  *      2: 0000000000000000FFFF0000020310AC:0016 0000000000000000FFFF00009DD8A9C3:A526 01 00000000:00000000 02:000A7214 00000000     0        0 2525 2 c732eca0 201 40 1 2 -1
  *
  */
-// TODO check what happens to the 'content' field of the hash
 void addtoconninode (char * buffer)
 {
 	short int sa_family;
@@ -64,15 +82,13 @@ void addtoconninode (char * buffer)
 
 	char rem_addr[128], local_addr[128];
 	int local_port, rem_port;
-    	struct sockaddr_in6 localaddr, remaddr;
-    	char addr6[INET6_ADDRSTRLEN];
     	struct in6_addr in6_local;
     	struct in6_addr in6_remote;
 
 	// the following leaks some memory.
 	unsigned long * inode = (unsigned long *) malloc (sizeof(unsigned long));
 
-	int matches = sscanf(buffer, "%*d: %64[0-9A-Fa-f]:%X %64[0-9A-Fa-f]:%X %*X %*lX:%*lX %*X:%*lX %*lX %*d %*d %ld %*512s\n",
+	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);
 
 	if (matches != 5) {
@@ -121,8 +137,8 @@ void addtoconninode (char * buffer)
 	else
 	{
 		/* this is an IPv4-style row */
-		sscanf(local_addr, "%X", &result_addr_local);
+		sscanf(local_addr, "%X", (unsigned int *) &result_addr_local);
-		sscanf(rem_addr, "%X",   &result_addr_remote);
+		sscanf(rem_addr, "%X",   (unsigned int *) &result_addr_remote);
 		sa_family = AF_INET;
 	}
 
@@ -140,7 +156,7 @@ void addtoconninode (char * buffer)
 
 	//std::cout << "Adding to conninode\n" << std::endl;
 
-	conninode->add(hashkey, (void *)inode);
+	conninode[hashkey] = inode;
 
 	/* workaround: sometimes, when a connection is actually from 172.16.3.1 to
 	 * 172.16.3.3, packages arrive from 195.169.216.157 to 172.16.3.3, where
@@ -150,7 +166,7 @@ void addtoconninode (char * buffer)
 	while (current_local_addr != NULL) {
 		/* TODO maybe only add the ones with the same sa_family */
 		snprintf(hashkey, HASHKEYSIZE * sizeof(char), "%s:%d-%s:%d", current_local_addr->string, local_port, remote_string, rem_port);
-		conninode->add(hashkey, (void *)inode);
+		conninode[hashkey] = inode;
 		current_local_addr = current_local_addr->next;
 	}
 	free (hashkey);
@@ -178,31 +194,49 @@ int addprocinfo (const char * filename) {
 	return 1;
 }
 
+std::map <unsigned long, prg_node *> inodeproc;
+
+/* this should be done quickly after the packet
+ * arrived, since the inode disappears from the table
+ * quickly, too :) */
 struct prg_node * findPID (unsigned long inode)
 {
-	/* find PID */
+	/* we first look in inodeproc */
-	struct prg_node * node = prg_cache_get(inode);
+	struct prg_node * node = inodeproc[inode];
+	
+	if (node != NULL)
+		return node;
+
+	node = prg_cache_get(inode);
 	if (node != NULL && node->pid == 1)
 	{
+		if (DEBUG)
+			std::cout << "ITP: clearing and reloading cache\n";
 		prg_cache_clear();
 		prg_cache_load();
 		node = prg_cache_get(inode);
 		// this still happens sometimes...
 		//assert (node->pid != 1);
-	}
+	} 
 
 	if (node == NULL)
 	{
+		if (DEBUG)
+			std::cout << "ITP: inode " << inode << " not in inode-to-pid-mapping - reloading." << endl;
 		prg_cache_clear();
 		prg_cache_load();
 		node = prg_cache_get(inode);
 		if (node == NULL)
 		{
 			if (DEBUG)
-				std::cout << "inode " << inode << " STILL not in inode-to-pid-mapping." << endl;
+				std::cout << "ITP: inode " << inode << " STILL not in inode-to-pid-mapping." << endl;
 			return NULL;
 		}
-	}	
+	} else if (DEBUG)
+		std::cout << "ITP: inode " << inode << " found in inode-to-pid-mapping." << endl;
+
+	inodeproc[inode] = node;
+
 	return node;
 }
 
@@ -219,6 +253,8 @@ Process * findProcess (struct prg_node * node)
 }
 
 /* finds process based on inode, if any */
+/* should be done quickly after arrival of the packet, 
+ * otherwise findPID will be outdated */
 Process * findProcess (unsigned long inode)
 {
 	struct prg_node * node = findPID(inode);
@@ -237,14 +273,16 @@ void reviewUnknown ()
 	ConnList * previous_conn = NULL;
 
 	while (curr_conn != NULL) {
-		unsigned long * inode = (unsigned long *)
+		unsigned long * inode = conninode[curr_conn->getVal()->refpacket->gethashstring()];
-			conninode->get(curr_conn->getVal()->refpacket->gethashstring());
 		if (inode != NULL)
 		{
 			Process * proc = findProcess (*inode);
 			if (proc != unknownproc && proc != NULL)
 			{
+				if (DEBUG)
+					std::cout << "ITP: WARNING: Previously unknown inode " << *inode << " now got process...??\n";
 				/* Yay! - but how could this happen? */
+				//assert(false);
 				if (previous_conn != NULL)
 				{
 					previous_conn->setNext (curr_conn->getNext());
@@ -297,6 +335,7 @@ char * uid2username (int uid)
 	pwd = getpwuid(uid);
 	if (pwd == NULL)
 	{
+		assert(false);
 		return strdup ("unlisted");
 	} else {
 		return strdup(pwd->pw_name);
@@ -306,7 +345,7 @@ char * uid2username (int uid)
 class Line 
 {
 public:
-	Line (const char * name, double n_sent_kbps, double n_recv_kbps, int pid, int uid, const char * n_devicename)
+	Line (const char * name, double n_sent_kbps, double n_recv_kbps, int pid, uid_t uid, const char * n_devicename)
 	{
 		m_name = name; 
 		sent_kbps = n_sent_kbps; 
@@ -314,6 +353,8 @@ public:
 		devicename = n_devicename;
 		m_pid = pid; 
 		m_uid = uid;
+		assert (m_uid >= 0);
+		assert (m_pid >= 0);
 	}
 
 	void show (int row);
@@ -331,6 +372,9 @@ void Line::show (int row)
 {
 	if (DEBUG || tracemode)
 	{
+		assert (m_uid >= 0);
+		assert (m_pid >= 0);
+
 		std::cout << m_name << '/' << m_pid << '/' << m_uid << "\t" << sent_kbps << "\t" << recv_kbps << std::endl;
 		return;
 	}
@@ -394,11 +438,19 @@ void do_refresh()
 	ProcList * curproc = processes;
 	ProcList * previousproc = NULL;
 	int nproc = count_processes();
+	/* initialise to null pointers */
 	Line * lines [nproc];
 	int n = 0, i = 0;
 	double sent_global = 0;
 	double recv_global = 0;
 
+	if (DEBUG)
+	{
+		// initialise to null pointers
+		for (int i = 0; i < nproc; i++)
+			lines[i] = NULL;
+	}
+
 	while (curproc != NULL)
 	{
 		// walk though its connections, summing up their data, and 
@@ -412,22 +464,25 @@ void do_refresh()
 		/* do not remove the unknown process */
 		if ((curproc->getVal()->getLastPacket() + PROCESSTIMEOUT <= curtime.tv_sec) && (curproc->getVal() != unknownproc))
 		{
-			/* remove connection */
+			/* remove process */
+			if (DEBUG)
+				std::cout << "PROC: Deleting process\n";
+			ProcList * todelete = curproc;
+			Process * p_todelete = curproc->getVal();
 			if (previousproc)
 			{
 				previousproc->next = curproc->next;
-				ProcList * newcur = curproc->next;
+				curproc = curproc->next;
-				delete curproc;
-				curproc = newcur;
-				nproc--;
 			} else {
 				processes = curproc->getNext();
-				delete curproc;
 				curproc = processes;
-				nproc--;
 			}
-			continue;
+			delete todelete;
+			delete p_todelete;
+			nproc--;
+			//continue;
 		}
+		else{
 
 		u_int32_t sum_sent = 0, 
 			  sum_recv = 0;
@@ -461,10 +516,16 @@ void do_refresh()
 				curconn = curconn->getNext();
 			}
 		}
-		lines[n] = new Line (curproc->getVal()->name, tokbps(sum_sent), tokbps(sum_recv), curproc->getVal()->pid, curproc->getVal()->uid, curproc->getVal()->devicename);
+		if (DEBUG) 
+		{
+			assert (curproc->getVal()->getUid() >= 0);
+		}
+		lines[n] = new Line (curproc->getVal()->name, tokbps(sum_sent), tokbps(sum_recv), 
+				curproc->getVal()->pid, curproc->getVal()->getUid(), curproc->getVal()->devicename);
 		previousproc = curproc;
 		curproc = curproc->next;
 		n++;
+		}
 	}
 
 	/* sort the accumulated lines */
@@ -523,13 +584,14 @@ Process * getProcess (unsigned long inode, char * devicename)
 	sprintf(procdir , "/proc/%d", node->pid);
 	struct stat stats;
 	stat(procdir, &stats);
-	newproc->uid = stats.st_uid;
+	newproc->setUid(stats.st_uid);
 
 	processes = new ProcList (newproc, processes);
 	return newproc;
 }
 
-/* Used when a new connection is encountered. Finds corresponding
+/* 
+ * Used when a new connection is encountered. Finds corresponding
  * process and adds the connection. If the connection  doesn't belong
  * to any known process, the process list is updated and a new process
  * is made. If no process can be found even then, it's added to the 
@@ -537,20 +599,16 @@ Process * getProcess (unsigned long inode, char * devicename)
  */
 Process * getProcess (Connection * connection, char * devicename)
 {
-	ProcList * curproc = processes;
+	unsigned long * inode = conninode[connection->refpacket->gethashstring()];
-
-	unsigned long * inode = (unsigned long *) 
-		conninode->get(connection->refpacket->gethashstring());
 
 	if (inode == NULL)
 	{
 		// no? refresh and check conn/inode table
 #if DEBUG
-		std::cerr << "new connection not in connection-to-inode table.\n"; 
+		std::cout << "LOC: new connection not in connection-to-inode table.\n"; 
 #endif
 		refreshconninode();
-		inode = (unsigned long *) 
+		inode = conninode[connection->refpacket->gethashstring()];
-			conninode->get(connection->refpacket->gethashstring());
 		if (inode == NULL)
 		{
 			/* HACK: the following is a hack for cases where the 
@@ -561,14 +619,13 @@ Process * getProcess (Connection * connection, char * devicename)
 			 * successful. */
 
 			Packet * reversepacket = connection->refpacket->newInverted();
-			inode = (unsigned long *) 
+			inode = conninode[reversepacket->gethashstring()];
-				conninode->get(reversepacket->gethashstring());
 
 			if (inode == NULL)
 			{
 				delete reversepacket;
 				if (DEBUG)
-					std::cout << connection->refpacket->gethashstring() << " STILL not in connection-to-inode table - adding to the unknown process\n";
+					std::cout << "LOC: " << connection->refpacket->gethashstring() << " STILL not in connection-to-inode table - adding to the unknown process\n";
 				unknownproc->connections = new ConnList (connection, unknownproc->connections);
 				return unknownproc;
 			}
@@ -585,6 +642,6 @@ Process * getProcess (Connection * connection, char * devicename)
 
 void procclean ()
 {
-	delete conninode;
+	//delete conninode;
 	prg_cache_clear();
 }

process.h

@@ -25,7 +25,7 @@ public:
 	{
 		return val;
 	}
-	ConnList * setNext (ConnList * m_next)
+	void setNext (ConnList * m_next)
 	{
 		next = m_next;
 	}
@@ -41,40 +41,49 @@ private:
 class Process
 {
 public:
+	/* the process makes a copy of the device name and name. */
 	Process (unsigned long m_inode, char * m_devicename, char * m_name = NULL)
 	{
+		if (DEBUG)
+			std::cout << "PROC: Process created at " << this << std::endl;
 		inode = m_inode;
-		name = m_name;
+
-		devicename = m_devicename;
+		if (m_name == NULL)
+			name = NULL;
+		else
+			name = strdup(m_name);
+
+		devicename = strdup(m_devicename);
 		connections = NULL;
 		pid = 0;
 		uid = 0;
 	}
-	int getLastPacket ()
+	/* TODO free m_name and m_devicename again in constructor */
+	~Process ()
 	{
-		int lastpacket=0;
+		if (DEBUG)
-		ConnList * curconn=connections;
+			std::cout << "PROC: Process deleted at " << this << std::endl;
-		while (curconn != NULL)
-		{
-			if (DEBUG)
-			{
-				assert (curconn != NULL);
-				assert (curconn->getVal() != NULL);
-			}
-			if (curconn->getVal()->getLastPacket() > lastpacket)
-				lastpacket = curconn->getVal()->getLastPacket();
-			curconn = curconn->getNext();
-		}
-		return lastpacket;
 	}
+	int getLastPacket ();
 
 	const char * name;
 	const char * devicename;
 	int pid;
-	int uid;
 
 	unsigned long inode;
 	ConnList * connections;
+	uid_t getUid()
+	{
+		return uid;
+	}
+
+	void setUid(uid_t m_uid)
+	{
+		assert (m_uid >= 0);
+		uid = m_uid;
+	}
+private:
+	uid_t uid;
 };
 
 Process * getProcess (Connection * connection, char * devicename = NULL);