-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Begin the process of conversion for IdentStateData to an AsyncJob. Referencing it as a 'job' below
* 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. Amos -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (MingW32) iQEcBAEBAgAGBQJUKmKfAAoJELJo5wb/XPRjp7MIAMyTlRRFS4Wc8juQkBEdFk2T XJtxm9ADG27CzkRhlbxs3XHqx7t9691ndBRUH2akwFWL7PElGX9YUKpH5E4q+SZe qka4f2wK+aL8+siX6uGWgkZQruUY75x/ZEcGyf9qBngt/zkDxfAULtTH4Jmp8pXl 8qo7ZgzccDfzXg7ojX3pHOeaaqZgpSzuPylw4f3h0x1Nr3o9sUb0UYCO8spxQpwz Q6tnmpTa0qxHusdRdDf7WtJHB3QU+5fEskwpKoTVTL8KoofBv/1z5eW4DphjSFmI e13AysTzXXKdMktyFBhyJPTRGRI8PjBrj+lSoA9rQLNXZz5BEhQOg6dIgT3d3z0= =8iNi -----END PGP SIGNATURE-----
=== modified file 'src/ident/Ident.cc'
--- src/ident/Ident.cc 2014-09-13 13:59:43 +0000
+++ src/ident/Ident.cc 2014-09-29 13:36:32 +0000
@@ -19,236 +19,259 @@
#include "CommCalls.h"
#include "globals.h"
#include "ident/Config.h"
#include "ident/Ident.h"
#include "MemBuf.h"
namespace Ident
{
#define IDENT_PORT 113
#define IDENT_KEY_SZ 50
#define IDENT_BUFSIZE 4096
typedef struct _IdentClient {
IDCB *callback;
void *callback_data;
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;
static IOCB WriteFeedback;
static CLCB Close;
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 (!Comm::IsConnOpen(conn)) {
+ comm_remove_close_handler(conn->fd, Ident::Close, this);
+ conn->close();
+ }
- if (result && *result == '\0')
- result = NULL;
+ queryMsg.clean();
- 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);
xfree(client);
}
}
void
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
Ident::ConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int
xerrno, void *data)
{
IdentStateData *state = (IdentStateData *)data;
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
*/
IdentClient *c;
for (c = state->clients; c; c = c->next) {
if (cbdataReferenceValid(c->callback_data))
break;
}
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",
CommIoCbPtrFun(Ident::WriteFeedback,
state));
Comm::Write(conn, &state->queryMsg, writeCall);
AsyncCall::Pointer readCall = commCbCall(5,4, "Ident::ReadReply",
CommIoCbPtrFun(Ident::ReadReply, state));
comm_read(conn, state->buf, IDENT_BUFSIZE, readCall);
AsyncCall::Pointer timeoutCall = commCbCall(5,4, "Ident::Timeout",
CommTimeoutCbPtrFun(Ident::Timeout,
state));
commSetConnTimeout(conn, Ident::TheConfig.timeout, timeoutCall);
}
void
Ident::WriteFeedback(const Comm::ConnectionPointer &conn, char *buf, size_t
len, Comm::Flag flag, int xerrno, void *data)
{
debugs(30, 5, HERE << conn << ": Wrote IDENT request " << len << "
bytes.");
// 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");
}
}
void
Ident::ReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len,
Comm::Flag flag, int xerrno, void *data)
{
IdentStateData *state = (IdentStateData *)data;
char *ident = NULL;
char *t = NULL;
assert(buf == state->buf);
assert(conn->fd == state->conn->fd);
if (flag != Comm::OK || len <= 0) {
- state->conn->close();
+ state->deleteThis("read error");
return;
}
/*
* XXX This isn't really very tolerant. It should read until EOL
* or EOF and then decode the answer... If the reply is fragmented
* then this will fail
*/
buf[len] = '\0';
if ((t = strchr(buf, '\r')))
*t = '\0';
if ((t = strchr(buf, '\n')))
*t = '\0';
debugs(30, 5, HERE << conn << ": Read '" << buf << "'");
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
Ident::ClientAdd(IdentStateData * state, IDCB * callback, void *callback_data)
{
IdentClient *c = (IdentClient *)xcalloc(1, sizeof(*c));
IdentClient **C;
c->callback = callback;
c->callback_data = cbdataReference(callback_data);
for (C = &state->clients; *C; C = &(*C)->next);
*C = c;
}
-CBDATA_TYPE(IdentStateData);
-
-/**** PUBLIC FUNCTIONS ****/
-
/*
* start a TCP connection to the peer host on port 113
*/
void
Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data)
{
IdentStateData *state;
char key1[IDENT_KEY_SZ];
char key2[IDENT_KEY_SZ];
char key[IDENT_KEY_SZ];
conn->local.toUrl(key1, IDENT_KEY_SZ);
conn->remote.toUrl(key2, IDENT_KEY_SZ);
snprintf(key, IDENT_KEY_SZ, "%s,%s", key1, key2);
if (!ident_hash) {
Init();
}
if ((state = (IdentStateData *)hash_lookup(ident_hash, key)) != NULL) {
ClientAdd(state, callback, 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.
state->conn = conn->copyDetails();
// NP: use random port for secure outbound to IDENT_PORT
state->conn->local.port(0);
state->conn->remote.port(IDENT_PORT);
// build our query from the original connection details
state->queryMsg.init();
state->queryMsg.Printf("%d, %d\r\n", conn->remote.port(),
conn->local.port());
ClientAdd(state, callback, data);
hash_join(ident_hash, &state->hash);
AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone",
CommConnectCbPtrFun(Ident::ConnectDone, state));
AsyncJob::Start(new Comm::ConnOpener(state->conn, call,
Ident::TheConfig.timeout));
}
void
ident_mk1_s35.patch.sig
Description: Binary data
_______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
