------------------------------------------------------------ revno: 13180 revision-id: squid3@treenet.co.nz-20141016192237-y1dxyuu7j62god5p parent: squid3@treenet.co.nz-20141016185603-b1boi62a0xq55tob fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=3803 committer: Amos Jeffries branch nick: 3.4 timestamp: Thu 2014-10-16 12:22:37 -0700 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20141016192237-y1dxyuu7j62god5p # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.4 # testament_sha1: 2623f3328523ae828a6ca23de75e371030d2ab90 # timestamp: 2014-10-16 19:50:38 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.4 # base_revision_id: squid3@treenet.co.nz-20141016185603-\ # b1boi62a0xq55tob # # Begin patch === modified file 'src/ident/Ident.cc' --- src/ident/Ident.cc 2014-08-03 11:56:37 +0000 +++ src/ident/Ident.cc 2014-10-16 19:22:37 +0000 @@ -56,13 +56,27 @@ 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 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::identCallback(IdentStateData * state, char *result) -{ - IdentClient *client; - - if (result && *result == '\0') - result = NULL; - - while ((client = state->clients)) { +void +Ident::IdentStateData::deleteThis(const char *aReason) +{ + swanSong(); + delete this; +} + +void +Ident::IdentStateData::swanSong() +{ + if (clients != NULL) + notify(NULL); + + if (Comm::IsConnOpen(conn)) { + comm_remove_close_handler(conn->fd, Ident::Close, this); + conn->close(); + } + + 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 @@ Ident::Close(const CommCloseCbParams ¶ms) { 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 @@ 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 @@ } 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 @@ // 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 @@ assert(conn->fd == state->conn->fd); if (flag != COMM_OK || len <= 0) { - state->conn->close(); + state->deleteThis("read error"); return; } @@ -204,11 +228,13 @@ 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 @@ *C = c; } -CBDATA_TYPE(IdentStateData); - -/**** PUBLIC FUNCTIONS ****/ - /* * start a TCP connection to the peer host on port 113 */ @@ -250,8 +272,7 @@ 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.