From a142648f45cb8261faf82279fa162f557e75be32 Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Sat, 15 Jan 2005 15:03:20 +0000 Subject: [PATCH] Fixed memory leaks --- Makefile | 2 +- connection.cpp | 2 + connection.h | 3 ++ cui.cpp | 115 +++++++++++++++++++++++++++++-------------------- inode2prog.cpp | 16 ++++++- nethogs.cpp | 2 +- process.cpp | 32 +++++++------- process.h | 8 ++-- 8 files changed, 112 insertions(+), 68 deletions(-) diff --git a/Makefile b/Makefile index 3d4eb43..daa0d99 100644 --- a/Makefile +++ b/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++ diff --git a/connection.cpp b/connection.cpp index 22b0906..1ee79a9 100644 --- a/connection.cpp +++ b/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) diff --git a/connection.h b/connection.h index d913eef..cdf8d03 100644 --- a/connection.h +++ b/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 diff --git a/cui.cpp b/cui.cpp index a482259..99d679f 100644 --- a/cui.cpp +++ b/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); - } - 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(); + 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(); + } + } + uid_t uid = curproc->getVal()->getUid(); + if (ROBUST) + { + 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++; } } diff --git a/inode2prog.cpp b/inode2prog.cpp index 40b2848..d7ee3ad 100644 --- a/inode2prog.cpp +++ b/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'; diff --git a/nethogs.cpp b/nethogs.cpp index eae5b74..7afda79 100644 --- a/nethogs.cpp +++ b/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) { diff --git a/process.cpp b/process.cpp index 503b2a9..3e65d19 100644 --- a/process.cpp +++ b/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 conninode; +std::map 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; } diff --git a/process.h b/process.h index 331c1d2..e9fd6fe 100644 --- a/process.h +++ b/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;