From cd3fce3e5485c9f7810f1fb68f9b2f0a01a03065 Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Wed, 15 Sep 2004 12:49:05 +0000 Subject: [PATCH] hunting memory management bug --- DESIGN | 2 + Makefile | 2 +- hashtbl.cpp | 2 +- nethogs.cpp | 2 +- nethogs.h | 2 +- packet.cpp | 7 ++- process.cpp | 137 +++++++++++++++++++++++++++++++++++++--------------- process.h | 47 ++++++++++-------- 8 files changed, 134 insertions(+), 67 deletions(-) diff --git a/DESIGN b/DESIGN index d6d21d7..e6682ff 100644 --- a/DESIGN +++ b/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. diff --git a/Makefile b/Makefile index 9a47e9f..c075e68 100644 --- a/Makefile +++ b/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 diff --git a/hashtbl.cpp b/hashtbl.cpp index 9a699d9..ac0aced 100644 --- a/hashtbl.cpp +++ b/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]); } diff --git a/nethogs.cpp b/nethogs.cpp index 41c38c8..6b32888 100644 --- a/nethogs.cpp +++ b/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) diff --git a/nethogs.h b/nethogs.h index 497f39c..23ab233 100644 --- a/nethogs.h +++ b/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 '-' diff --git a/packet.cpp b/packet.cpp index fcb6494..d6be29a 100644 --- a/packet.cpp +++ b/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) - std::cout << "Returning newly created hash string: " << hashstring << std::endl; + //if (DEBUG) + // std::cout << "Returning newly created hash string: " << hashstring << std::endl; return hashstring; } diff --git a/process.cpp b/process.cpp index 7e24d58..e5a15a6 100644 --- a/process.cpp +++ b/process.cpp @@ -8,12 +8,12 @@ #include #include #include +#include #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 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(rem_addr, "%X", &result_addr_remote); + sscanf(local_addr, "%X", (unsigned int *) &result_addr_local); + 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 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 */ - struct prg_node * node = prg_cache_get(inode); + /* we first look in inodeproc */ + 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 *) - conninode->get(curr_conn->getVal()->refpacket->gethashstring()); + unsigned long * inode = conninode[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; - delete curproc; - curproc = newcur; - nproc--; + curproc = curproc->next; } 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 = (unsigned long *) - conninode->get(connection->refpacket->gethashstring()); + unsigned long * inode = conninode[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 *) - conninode->get(connection->refpacket->gethashstring()); + inode = conninode[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 *) - conninode->get(reversepacket->gethashstring()); + inode = conninode[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(); } diff --git a/process.h b/process.h index e0c3ad0..546055f 100644 --- a/process.h +++ b/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; - 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; + if (DEBUG) + std::cout << "PROC: Process deleted at " << this << std::endl; } + 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);