From: Amos Jeffries <squid3@treenet.co.nz>
Date: Thu, 16 Oct 2014 19:22:37 +0000 (-0700)
Subject: Bug 3803: ident leaks memory on failure
X-Git-Tag: SQUID_3_4_9~7
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d340e35ebcd5d31a3ba1ecffe755ef2c5fb99569;p=thirdparty%2Fsquid.git

Bug 3803: ident leaks memory on failure

Begin the process of conversion for IdentStateData to an AsyncJob.

* convert the object from CBDATA struct to a class with
CBDATA_CLASS2() API.

* Bug 3803 is caused by a lack of proper cleanup and consistent exit
actions terminating the job. Take the core logic changes from the
tested bug patch and;

 1) define a swanSong() method to cleanup the memory allocated

 2) define a deleteThis() method to emulate AsyncJob::deleteThis()

* Locate all code paths leveraging conn->close() to trigger cleanup
via the connection close handler and convert to explicit deleteThis()
with excuse. Including a few which were not but need to in order to
terminate the job correctly as fixed in bug 3803 patch.

The actions performed are nearly identical to the original code. The
differences are that many code paths now omit an AsyncCall step going
via the Comm close handler, and that all paths terminating the IDENT
lookup now go through swanSong() cleanup.


Further cleanup converting to a full AsyncJob is not included, since
there is an explicit hash of running IdentStateData object pointers
being used in the old code.
---

diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc
index 4d705eafb3..d54131169d 100644
--- a/src/ident/Ident.cc
+++ b/src/ident/Ident.cc
@@ -56,13 +56,27 @@ typedef struct _IdentClient {
     struct _IdentClient *next;
 } IdentClient;
 
-typedef struct _IdentStateData {
+class IdentStateData
+{
+public:
+    /* AsyncJob API emulated */
+    void deleteThis(const char *aReason);
+    void swanSong();
+
+    /// notify all waiting IdentClient callbacks
+    void notify(const char *result);
+
     hash_link hash;		/* must be first */
     Comm::ConnectionPointer conn;
     MemBuf queryMsg;  ///< the lookup message sent to IDENT server
     IdentClient *clients;
     char buf[IDENT_BUFSIZE];
-} IdentStateData;
+
+private:
+    CBDATA_CLASS2(IdentStateData);
+};
+
+CBDATA_CLASS_INIT(IdentStateData);
 
 // TODO: make these all a series of Async job calls. They are self-contained callbacks now.
 static IOCB ReadReply;
@@ -72,25 +86,39 @@ static CTCB Timeout;
 static CNCB ConnectDone;
 static hash_table *ident_hash = NULL;
 static void ClientAdd(IdentStateData * state, IDCB * callback, void *callback_data);
-static void identCallback(IdentStateData * state, char *result);
 
 } // namespace Ident
 
 Ident::IdentConfig Ident::TheConfig;
 
-/**** PRIVATE FUNCTIONS ****/
+void
+Ident::IdentStateData::deleteThis(const char *aReason)
+{
+    swanSong();
+    delete this;
+}
 
 void
-Ident::identCallback(IdentStateData * state, char *result)
+Ident::IdentStateData::swanSong()
 {
-    IdentClient *client;
+    if (clients != NULL)
+        notify(NULL);
 
-    if (result && *result == '\0')
-        result = NULL;
+    if (Comm::IsConnOpen(conn)) {
+        comm_remove_close_handler(conn->fd, Ident::Close, this);
+        conn->close();
+    }
 
-    while ((client = state->clients)) {
+    hash_remove_link(ident_hash, (hash_link *) this);
+    xfree(hash.key);
+}
+
+void
+Ident::IdentStateData::notify(const char *result)
+{
+    while (IdentClient *client = clients) {
         void *cbdata;
-        state->clients = client->next;
+        clients = client->next;
 
         if (cbdataReferenceValidDone(client->callback_data, &cbdata))
             client->callback(result, cbdata);
@@ -103,18 +131,15 @@ void
 Ident::Close(const CommCloseCbParams &params)
 {
     IdentStateData *state = (IdentStateData *)params.data;
-    identCallback(state, NULL);
-    state->conn->close();
-    hash_remove_link(ident_hash, (hash_link *) state);
-    xfree(state->hash.key);
-    cbdataFree(state);
+    state->deleteThis("connection closed");
 }
 
 void
 Ident::Timeout(const CommTimeoutCbParams &io)
 {
     debugs(30, 3, HERE << io.conn);
-    io.conn->close();
+    IdentStateData *state = (IdentStateData *)io.data;
+    state->deleteThis("timeout");
 }
 
 void
@@ -125,12 +150,10 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, comm_err_t status, int x
     if (status != COMM_OK) {
         if (status == COMM_TIMEOUT)
             debugs(30, 3, "IDENT connection timeout to " << state->conn->remote);
-        Ident::identCallback(state, NULL);
+        state->deleteThis(status == COMM_TIMEOUT ? "connect timeout" : "connect error");
         return;
     }
 
-    assert(conn != NULL && conn == state->conn);
-
     /*
      * see if any of our clients still care
      */
@@ -141,11 +164,11 @@ Ident::ConnectDone(const Comm::ConnectionPointer &conn, comm_err_t status, int x
     }
 
     if (c == NULL) {
-        /* no clients care */
-        conn->close();
+        state->deleteThis("client(s) aborted");
         return;
     }
 
+    assert(conn != NULL && conn == state->conn);
     comm_add_close_handler(conn->fd, Ident::Close, state);
 
     AsyncCall::Pointer writeCall = commCbCall(5,4, "Ident::WriteFeedback",
@@ -167,7 +190,8 @@ Ident::WriteFeedback(const Comm::ConnectionPointer &conn, char *buf, size_t len,
     // TODO handle write errors better. retry or abort?
     if (flag != COMM_OK) {
         debugs(30, 2, HERE << conn << " err-flags=" << flag << " IDENT write error: " << xstrerr(xerrno));
-        conn->close();
+        IdentStateData *state = (IdentStateData *)data;
+        state->deleteThis("write error");
     }
 }
 
@@ -182,7 +206,7 @@ Ident::ReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len, com
     assert(conn->fd == state->conn->fd);
 
     if (flag != COMM_OK || len <= 0) {
-        state->conn->close();
+        state->deleteThis("read error");
         return;
     }
 
@@ -204,11 +228,13 @@ Ident::ReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len, com
     if (strstr(buf, "USERID")) {
         if ((ident = strrchr(buf, ':'))) {
             while (xisspace(*++ident));
-            Ident::identCallback(state, ident);
+            if (ident && *ident == '\0')
+                ident = NULL;
+            state->notify(ident);
         }
     }
 
-    state->conn->close();
+    state->deleteThis("completed");
 }
 
 void
@@ -223,10 +249,6 @@ Ident::ClientAdd(IdentStateData * state, IDCB * callback, void *callback_data)
     *C = c;
 }
 
-CBDATA_TYPE(IdentStateData);
-
-/**** PUBLIC FUNCTIONS ****/
-
 /*
  * start a TCP connection to the peer host on port 113
  */
@@ -250,8 +272,7 @@ Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data)
         return;
     }
 
-    CBDATA_INIT_TYPE(IdentStateData);
-    state = cbdataAlloc(IdentStateData);
+    state = new IdentStateData;
     state->hash.key = xstrdup(key);
 
     // copy the conn details. We dont want the original FD to be re-used by IDENT.