Author: Henrik Nordstrom <henrik@henriknordstrom.net>

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 da6b2b84b41fa4ff6cd8409659eec502a35ed222..a03af00596b174d46e217c41a7f1db045828cb45 100644 (file)
--- a/src/auth/negotiate/auth_negotiate.cc
+++ b/src/auth/negotiate/auth_negotiate.cc
@@ -367,13 +367,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<authenticateStateData *>(data);
 
     int valid;
-    stateful_helper_callback_t result = S_HELPER_UNKNOWN;
     char *blob, *arg = NULL;
 
     AuthUserRequest *auth_user_request;
@@ -385,11 +384,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) {
@@ -433,18 +431,16 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
         if (arg)
             *arg++ = '\0';
         safe_free(negotiate_request->server_blob);
-       negotiate_request->request->flags.must_keepalive = 1;
-       if (negotiate_request->request->flags.proxy_keepalive) {
-           negotiate_request->server_blob = xstrdup(blob);
-           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;
-       }
+        negotiate_request->request->flags.must_keepalive = 1;
+        if (negotiate_request->request->flags.proxy_keepalive) {
+            negotiate_request->server_blob = xstrdup(blob);
+            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 << "'");
+        } else {
+            negotiate_request->auth_state = AUTHENTICATE_STATE_FAILED;
+            auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
+        }
     } else if (strncasecmp(reply, "AF ", 3) == 0 && arg != NULL) {
         /* we're finished, release the helper */
 
@@ -463,8 +459,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 */
@@ -510,8 +504,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
 
         authenticateNegotiateReleaseServer(negotiate_request);
 
-        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
@@ -523,7 +515,6 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
         negotiate_request->auth_state = AUTHENTICATE_STATE_FAILED;
         safe_free(negotiate_request->server_blob);
         authenticateNegotiateReleaseServer(negotiate_request);
-        result = S_HELPER_RELEASE;
         debugs(29, 1, "authenticateNegotiateHandleReply: Error validating user via Negotiate. Error returned '" << reply << "'");
     } else {
         /* protocol error */
@@ -537,8 +528,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 165633578f235213c38d5f5c51a7b365d97d3d01..8c1117e263fbfe7b34f9de581c74e95132ca7f40 100644 (file)
--- a/src/auth/ntlm/auth_ntlm.cc
+++ b/src/auth/ntlm/auth_ntlm.cc
@@ -329,13 +329,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<authenticateStateData *>(data);
 
     int valid;
-    stateful_helper_callback_t result = S_HELPER_UNKNOWN;
     char *blob;
 
     AuthUserRequest *auth_user_request;
@@ -347,11 +346,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) {
@@ -389,25 +387,22 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply)
     if (strncasecmp(reply, "TT ", 3) == 0) {
         /* we have been given a blob to send to the client */
         safe_free(ntlm_request->server_blob);
-       ntlm_request->request->flags.must_keepalive = 1;
-       if (ntlm_request->request->flags.proxy_keepalive) {
-           ntlm_request->server_blob = xstrdup(blob);
-           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;
-       }
+        ntlm_request->request->flags.must_keepalive = 1;
+        if (ntlm_request->request->flags.proxy_keepalive) {
+            ntlm_request->server_blob = xstrdup(blob);
+            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 << "'");
+        } else {
+            ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED;
+            auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
+        }
     } else if (strncasecmp(reply, "AF ", 3) == 0) {
         /* we're finished, release the helper */
         ntlm_user->username(blob);
         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());
@@ -441,7 +436,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply)
         ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED;
         safe_free(ntlm_request->server_blob);
         authenticateNTLMReleaseServer(ntlm_request);
-        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
@@ -453,7 +447,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply)
         ntlm_request->auth_state = AUTHENTICATE_STATE_FAILED;
         safe_free(ntlm_request->server_blob);
         authenticateNTLMReleaseServer(ntlm_request);
-        result = S_HELPER_RELEASE;
         debugs(29, 1, "authenticateNTLMHandleReply: Error validating user via NTLM. Error returned '" << reply << "'");
     } else {
         /* protocol error */
@@ -467,8 +460,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 959edaf6edc88816d019b30c0e93889516f713d9..269fb3d0a769071d78d176b920daea802e9d7888 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -991,6 +991,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')
@@ -999,36 +1000,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;
@@ -1042,7 +1017,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)
@@ -1309,6 +1287,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;
     }
 
@@ -1331,6 +1310,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 8de49dd7165038203a24d694bcfbd8c16065dd73..150c19b1aef3b84c4e3bb232b21f289d6ba10358 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -53,7 +53,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
 {