From: Henrik Nordstrom Date: Tue, 4 Aug 2009 14:39:10 +0000 (+0200) Subject: Bug #2648: stateful helpers stuck in reserved if client disconnects while helper... X-Git-Tag: SQUID_3_2_0_1~813 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dcb802aa7ae3d63d5897bbe9260abc6533f48717;p=thirdparty%2Fsquid.git Bug #2648: stateful helpers stuck in reserved if client disconnects while helper busy Note: May depend on the other stateful helper cleanups. --- diff --git a/src/auth/negotiate/auth_negotiate.cc b/src/auth/negotiate/auth_negotiate.cc index 81dcc9298f..a3cf1fdf97 100644 --- a/src/auth/negotiate/auth_negotiate.cc +++ b/src/auth/negotiate/auth_negotiate.cc @@ -379,13 +379,12 @@ NegotiateUser::~NegotiateUser() debugs(29, 5, "NegotiateUser::~NegotiateUser: doing nothing to clearNegotiate scheme data for '" << this << "'"); } -static stateful_helper_callback_t +static void authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) { authenticateStateData *r = static_cast(data); int valid; - stateful_helper_callback_t result = S_HELPER_UNKNOWN; char *blob, *arg = NULL; AuthUserRequest *auth_user_request; @@ -397,11 +396,10 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) valid = cbdataReferenceValid(r->data); if (!valid) { - debugs(29, 1, "authenticateNegotiateHandleReply: invalid callback data. Releasing helper '" << lastserver << "'."); + debugs(29, 1, "authenticateNegotiateHandleReply: invalid callback data. helper '" << lastserver << "'."); cbdataReferenceDone(r->data); authenticateStateFree(r); - debugs(29, 9, "authenticateNegotiateHandleReply: telling stateful helper : " << S_HELPER_RELEASE); - return S_HELPER_RELEASE; + return; } if (!reply) { @@ -451,11 +449,9 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) negotiate_request->auth_state = AUTHENTICATE_STATE_IN_PROGRESS; auth_user_request->denyMessage("Authentication in progress"); debugs(29, 4, "authenticateNegotiateHandleReply: Need to challenge the client with a server blob '" << blob << "'"); - result = S_HELPER_RESERVE; } else { negotiate_request->auth_state = AUTHENTICATE_STATE_FAILED; auth_user_request->denyMessage("NTLM authentication requires a persistent connection"); - result = S_HELPER_RELEASE; } } else if (strncasecmp(reply, "AF ", 3) == 0 && arg != NULL) { /* we're finished, release the helper */ @@ -475,8 +471,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) negotiate_request->auth_state = AUTHENTICATE_STATE_DONE; - result = S_HELPER_RELEASE; - debugs(29, 4, "authenticateNegotiateHandleReply: Successfully validated user via Negotiate. Username '" << blob << "'"); /* connection is authenticated */ @@ -522,8 +516,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) negotiate_request->releaseAuthServer(); - result = S_HELPER_RELEASE; - debugs(29, 4, "authenticateNegotiateHandleReply: Failed validating user via Negotiate. Error returned '" << blob << "'"); } else if (strncasecmp(reply, "BH ", 3) == 0) { /* TODO kick off a refresh process. This can occur after a YR or after @@ -535,7 +527,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) negotiate_request->auth_state = AUTHENTICATE_STATE_FAILED; safe_free(negotiate_request->server_blob); negotiate_request->releaseAuthServer(); - result = S_HELPER_RELEASE; debugs(29, 1, "authenticateNegotiateHandleReply: Error validating user via Negotiate. Error returned '" << reply << "'"); } else { /* protocol error */ @@ -549,8 +540,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply) r->handler(r->data, NULL); cbdataReferenceDone(r->data); authenticateStateFree(r); - debugs(29, 9, "authenticateNegotiateHandleReply: telling stateful helper : " << result); - return result; } static void diff --git a/src/auth/ntlm/auth_ntlm.cc b/src/auth/ntlm/auth_ntlm.cc index 0d18a841db..3f4d0c38bb 100644 --- a/src/auth/ntlm/auth_ntlm.cc +++ b/src/auth/ntlm/auth_ntlm.cc @@ -325,13 +325,12 @@ NTLMUser::~NTLMUser() debugs(29, 5, "NTLMUser::~NTLMUser: doing nothing to clearNTLM scheme data for '" << this << "'"); } -static stateful_helper_callback_t +static void authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) { authenticateStateData *r = static_cast(data); int valid; - stateful_helper_callback_t result = S_HELPER_UNKNOWN; char *blob; AuthUserRequest *auth_user_request; @@ -343,11 +342,10 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) valid = cbdataReferenceValid(r->data); if (!valid) { - debugs(29, 1, "authenticateNTLMHandleReply: invalid callback data. Releasing helper '" << lastserver << "'."); + debugs(29, 1, "authenticateNTLMHandleReply: invalid callback data. helper '" << lastserver << "'."); cbdataReferenceDone(r->data); authenticateStateFree(r); - debugs(29, 9, "authenticateNTLMHandleReply: telling stateful helper : " << S_HELPER_RELEASE); - return S_HELPER_RELEASE; + return; } if (!reply) { @@ -391,11 +389,9 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) ntlm_request->auth_state = AUTHENTICATE_STATE_IN_PROGRESS; auth_user_request->denyMessage("Authentication in progress"); debugs(29, 4, "authenticateNTLMHandleReply: Need to challenge the client with a server blob '" << blob << "'"); - result = S_HELPER_RESERVE; } else { ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED; auth_user_request->denyMessage("NTLM authentication requires a persistent connection"); - result = S_HELPER_RELEASE; } } else if (strncasecmp(reply, "AF ", 3) == 0) { /* we're finished, release the helper */ @@ -403,7 +399,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) auth_user_request->denyMessage("Login successful"); safe_free(ntlm_request->server_blob); - result = S_HELPER_RELEASE; debugs(29, 4, "authenticateNTLMHandleReply: Successfully validated user via NTLM. Username '" << blob << "'"); /* connection is authenticated */ debugs(29, 4, "AuthNTLMUserRequest::authenticate: authenticated user " << ntlm_user->username()); @@ -437,7 +432,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED; safe_free(ntlm_request->server_blob); ntlm_request->releaseAuthServer(); - result = S_HELPER_RELEASE; debugs(29, 4, "authenticateNTLMHandleReply: Failed validating user via NTLM. Error returned '" << blob << "'"); } else if (strncasecmp(reply, "BH ", 3) == 0) { /* TODO kick off a refresh process. This can occur after a YR or after @@ -449,7 +443,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED; safe_free(ntlm_request->server_blob); ntlm_request->releaseAuthServer(); - result = S_HELPER_RELEASE; debugs(29, 1, "authenticateNTLMHandleReply: Error validating user via NTLM. Error returned '" << reply << "'"); } else { /* protocol error */ @@ -463,8 +456,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) r->handler(r->data, NULL); cbdataReferenceDone(r->data); authenticateStateFree(r); - debugs(29, 9, "authenticateNTLMHandleReply: telling stateful helper : " << result); - return result; } static void diff --git a/src/helper.cc b/src/helper.cc index 577cdca312..8f4c6db3e4 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -998,6 +998,7 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer if ((t = strchr(srv->rbuf, '\n'))) { /* end of reply found */ + int called = 1; debugs(84, 3, "helperStatefulHandleRead: end of reply found"); if (t > srv->rbuf && t[-1] == '\r') @@ -1006,36 +1007,10 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer *t = '\0'; if (r && cbdataReferenceValid(r->data)) { - switch ((r->callback(r->data, srv, srv->rbuf))) { /*if non-zero reserve helper */ - - case S_HELPER_UNKNOWN: - fatal("helperStatefulHandleRead: either a non-state aware callback was give to the stateful helper routines, or an uninitialised callback response was received.\n"); - break; - - case S_HELPER_RELEASE: /* helper finished with */ - - srv->flags.reserved = 0; - - if ((srv->parent->OnEmptyQueue != NULL) && (srv->data)) - srv->parent->OnEmptyQueue(srv->data); - - debugs(84, 5, "StatefulHandleRead: releasing " << hlp->id_name << " #" << srv->index + 1); - - break; - - case S_HELPER_RESERVE: /* 'pin' this helper for the caller */ - - srv->flags.reserved = 1; - debugs(84, 5, "StatefulHandleRead: reserving " << hlp->id_name << " #" << srv->index + 1); - - break; - - default: - fatal("helperStatefulHandleRead: unknown stateful helper callback result.\n"); - } - + r->callback(r->data, srv, srv->rbuf); } else { debugs(84, 1, "StatefulHandleRead: no callback data registered"); + called = 0; } srv->flags.busy = 0; @@ -1049,7 +1024,10 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer tvSubMsec(srv->dispatch_time, current_time), hlp->stats.replies, REDIRECT_AV_FACTOR); - helperStatefulServerDone(srv); + if (called) + helperStatefulServerDone(srv); + else + helperStatefulReleaseServer(srv); } if (srv->rfd != -1) @@ -1316,6 +1294,7 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r if (!cbdataReferenceValid(r->data)) { debugs(84, 1, "helperStatefulDispatch: invalid callback data"); helperStatefulRequestFree(r); + helperStatefulReleaseServer(srv); return; } @@ -1338,6 +1317,7 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r } srv->flags.busy = 1; + srv->flags.reserved = 1; srv->request = r; srv->dispatch_time = current_time; comm_write(srv->wfd, diff --git a/src/helper.h b/src/helper.h index d97a572994..f860b9305e 100644 --- a/src/helper.h +++ b/src/helper.h @@ -51,7 +51,7 @@ typedef struct _helper_flags helper_flags; typedef struct _helper_stateful_flags helper_stateful_flags; -typedef stateful_helper_callback_t HLPSCB(void *, void *lastserver, char *buf); +typedef void HLPSCB(void *, void *lastserver, char *buf); struct _helper { wordlist *cmdline;