From: Amos Jeffries <squid3@treenet.co.nz>
Date: Sun, 19 Sep 2010 03:03:02 +0000 (+1200)
Subject: Several leaks, nul-pointer dereferences and redundant checks
X-Git-Tag: take1~250
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b115733cbee4800a425a0cd0ec3933ac5ba06724;p=thirdparty%2Fsquid.git

Several leaks, nul-pointer dereferences and redundant checks

Caught by cppcheck static analysis.
---

diff --git a/src/disk.cc b/src/disk.cc
index 44dbf5567a..85936a4ed0 100644
--- a/src/disk.cc
+++ b/src/disk.cc
@@ -166,12 +166,8 @@ file_close(int fd)
  * select() loop.       --SLF
  */
 static void
-
 diskCombineWrites(struct _fde_disk *fdd)
 {
-    int len = 0;
-    dwrite_q *q = NULL;
-    dwrite_q *wq = NULL;
     /*
      * We need to combine multiple write requests on an FD's write
      * queue But only if we don't need to seek() in between them, ugh!
@@ -179,12 +175,12 @@ diskCombineWrites(struct _fde_disk *fdd)
      */
 
     if (fdd->write_q != NULL && fdd->write_q->next != NULL) {
-        len = 0;
+        int len = 0;
 
-        for (q = fdd->write_q; q != NULL; q = q->next)
+        for (dwrite_q *q = fdd->write_q; q != NULL; q = q->next)
             len += q->len - q->buf_offset;
 
-        wq = (dwrite_q *)memAllocate(MEM_DWRITE_Q);
+        dwrite_q *wq = (dwrite_q *)memAllocate(MEM_DWRITE_Q);
 
         wq->buf = (char *)xmalloc(len);
 
@@ -196,8 +192,9 @@ diskCombineWrites(struct _fde_disk *fdd)
 
         wq->free_func = cxx_xfree;
 
-        do {
-            q = fdd->write_q;
+        while (fdd->write_q != NULL) {
+            dwrite_q *q = fdd->write_q;
+
             len = q->len - q->buf_offset;
             xmemcpy(wq->buf + wq->len, q->buf + q->buf_offset, len);
             wq->len += len;
@@ -206,11 +203,8 @@ diskCombineWrites(struct _fde_disk *fdd)
             if (q->free_func)
                 (q->free_func) (q->buf);
 
-            if (q) {
-                memFree(q, MEM_DWRITE_Q);
-                q = NULL;
-            }
-        } while (fdd->write_q != NULL);
+            memFree(q, MEM_DWRITE_Q);
+        };
 
         fdd->write_q_tail = wq;
 
diff --git a/src/fde.h b/src/fde.h
index ebfbd36022..a6624e1550 100644
--- a/src/fde.h
+++ b/src/fde.h
@@ -52,6 +52,7 @@ public:
     bool readPending(int);
     void noteUse(PconnPool *);
 
+public:
     unsigned int type;
     u_short remote_port;
 
@@ -61,7 +62,7 @@ public:
     char ipaddr[MAX_IPSTRLEN];            /* dotted decimal address of peer */
     char desc[FD_DESC_SZ];
 
-    struct {
+    struct _fde_flags {
         unsigned int open:1;
         unsigned int close_request:1; // file_ or comm_close has been called
         unsigned int write_daemon:1;
@@ -116,14 +117,43 @@ public:
 private:
     /** Clear the fde class back to NULL equivalent. */
     inline void clear() {
+        type = 0;
+        remote_port = 0;
+        local_addr.SetEmpty();
+        tos = '\0';
+        sock_family = 0;
+        memset(ipaddr, '\0', MAX_IPSTRLEN);
+        memset(desc,'\0',FD_DESC_SZ);
+        memset(flags,0,sizeof(_fde_flags));
+        bytes_read = 0;
+        bytes_written = 0;
+        pconn.uses = 0;
+        pconn.pool = NULL;
+        epoll_state = 0;
+        memset(disk, 0, sizeof(_fde_disk));
+        read_handler = NULL;
+        read_data = NULL;
+        write_handler = NULL;
+        write_data = NULL;
         timeoutHandler = NULL;
+        timeout = 0;
+        writeStart = 0;
+        lifetime_data = NULL;
         closeHandler = NULL;
         halfClosedReader = NULL;
-        // XXX: the following memset may corrupt or leak new or changed members
-        memset(this, 0, sizeof(fde));
-        local_addr.SetEmpty(); // Ip::Address likes to be setup nicely.
+        wstate = NULL;
+        read_method = NULL;
+        write_method = NULL;
+#if USE_SSL
+        ssl = NULL;
+#endif
+#ifdef _SQUID_MSWIN_
+        win32.handle = NULL;
+#endif
+#if USE_ZPH_QOS
+        upstreamTOS = 0;
+#endif
     }
-
 };
 
 SQUIDCEXTERN int fdNFree(void);
diff --git a/src/ftp.cc b/src/ftp.cc
index f2ab342483..bd82d92205 100644
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -1847,7 +1847,9 @@ FtpStateData::loginFailed()
         } else if (ctrl.replycode == 421) {
             err = errorCon(ERR_FTP_UNAVAILABLE, HTTP_SERVICE_UNAVAILABLE, fwd->request);
         }
-    } else {
+    }
+
+    if (err) {
         ftpFail(this);
         return;
     }
diff --git a/src/icp_v2.cc b/src/icp_v2.cc
index 22e4f4c82d..692eed66b2 100644
--- a/src/icp_v2.cc
+++ b/src/icp_v2.cc
@@ -235,15 +235,13 @@ void
 icpUdpSendQueue(int fd, void *unused)
 {
     icpUdpData *q;
-    int x;
-    int delay;
 
     while ((q = IcpQueueHead) != NULL) {
-        delay = tvSubUsec(q->queue_time, current_time);
+        int delay = tvSubUsec(q->queue_time, current_time);
         /* increment delay to prevent looping */
-        x = icpUdpSend(fd, q->address, (icp_common_t *) q->msg, q->logcode, ++delay);
+        const int x = icpUdpSend(fd, q->address, (icp_common_t *) q->msg, q->logcode, ++delay);
         IcpQueueHead = q->next;
-        safe_free(q);
+        xfree(q);
 
         if (x < 0)
             break;
diff --git a/src/neighbors.cc b/src/neighbors.cc
index 48cad03eb7..d0b26e80a2 100644
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -1205,17 +1205,15 @@ peerDestroy(void *data)
 {
     peer *p = (peer *)data;
 
-    struct _domain_ping *l = NULL;
-
-    struct _domain_ping *nl = NULL;
-
     if (p == NULL)
         return;
 
-    for (l = p->peer_domain; l; l = nl) {
+    struct _domain_ping *nl = NULL;
+
+    for (struct _domain_ping *l = p->peer_domain; l; l = nl) {
         nl = l->next;
         safe_free(l->domain);
-        safe_free(l);
+        xfree(l);
     }
 
     safe_free(p->host);