Bug #2648: stateful helpers stuck in reserved if client disconnects while helper...

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 81dcc9298f2c6812e574ef6acbae33e2a92b1b88..a3cf1fdf97b79d84c08a3e91e36271642e1a4a5e 100644 (file)
--- 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<authenticateStateData *>(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 0d18a841db971df67e6741bdbb047044dc135ad8..3f4d0c38bb7bdc128aa53e791f1862606dd871b9 100644 (file)
--- 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<authenticateStateData *>(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 577cdca3129593ebf28fa159eeae7364dbcf0d72..8f4c6db3e4aa9e379e71687f2b2f4fd06c55c59f 100644 (file)
--- 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 d97a572994f48a142bd21c72b840d8b3170dbe3b..f860b9305e430f2d46454dc94bbd51f86eb13810 100644 (file)
--- 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;