From: wessels <> Date: Wed, 9 May 2007 15:07:38 +0000 (+0000) Subject: Bug #1951: NTLM authentication does not work X-Git-Tag: SQUID_3_0_PRE6~3 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4f0ef8e8d2e9f694437e2c1b12649a759bd7bb26;p=thirdparty%2Fsquid.git Bug #1951: NTLM authentication does not work Turns out there were a number of reasons why NTLM authentication didn't work. Some of these were addressed by patches committed a few revs earlier. With this current patch, Squid is now handling a Web Polygraph workload with NTLM authentication. The big change here is to locking of AuthUserRequest by the various other classes that maintain a pointer to it. The old locking logic was difficult for me to follow, and it seemed that there were not enough unlocks, leading to helper processes getting stuck in RESERVED state. I copied the ideas from HttpMsg locking and created macros AUTHUSERREQUESTLOCK and AUTHUSERREQUESTUNLOCK. I also tried to make sure that locks and unlocks occur in places where pointers are assigned. The most tricky part is with the ACLchecklist class, which passes a pointer to a pointer. Previously AuthUserRequest was doing a lot of locking on behalf of ACLchecklist, but now I make ACLchecklist do all its own locking and AuthUserRequest really shouldn't lock its own objects for any other classes. Another important change was to helperStatefulReleaseServer(). The old version apparently ignored helper servers in the S_HELPER_RESERVED states. It was only concerned about the S_HELPER_DEFERRED state. Now I think it does the right thing for both states. This patch also contains numerous cosmetic "style" changes to the source code and debugging that don't really affect its functionality. I couldn't resist. --- diff --git a/src/ACLChecklist.cc b/src/ACLChecklist.cc index 564dabb293..6b88879f52 100644 --- a/src/ACLChecklist.cc +++ b/src/ACLChecklist.cc @@ -1,5 +1,5 @@ /* - * $Id: ACLChecklist.cc,v 1.37 2007/05/09 08:31:47 wessels Exp $ + * $Id: ACLChecklist.cc,v 1.38 2007/05/09 09:07:38 wessels Exp $ * * DEBUG: section 28 Access Control * AUTHOR: Duane Wessels @@ -63,7 +63,24 @@ ACLChecklist::authenticated() /* get authed here */ /* Note: this fills in auth_user_request when applicable */ - switch (AuthUserRequest::tryToAuthenticateAndSetAuthUser (&auth_user_request, headertype, request, conn(), src_addr)) { + /* + * DPW 2007-05-08 + * tryToAuthenticateAndSetAuthUser used to try to lock and + * unlock auth_user_request on our behalf, but it was too + * ugly and hard to follow. Now we do our own locking here. + * + * I'm not sure what tryToAuthenticateAndSetAuthUser does when + * auth_user_request is set before calling. I'm tempted to + * unlock and set it to NULL, but it seems safer to save the + * pointer before calling and unlock it afterwards. If the + * pointer doesn't change then its a no-op. + */ + AuthUserRequest *old_auth_user_request = auth_user_request; + auth_acl_t result = AuthUserRequest::tryToAuthenticateAndSetAuthUser (&auth_user_request, headertype, request, conn(), src_addr); + if (auth_user_request) + AUTHUSERREQUESTLOCK(auth_user_request, "ACLChecklist"); + AUTHUSERREQUESTUNLOCK(old_auth_user_request, "old ACLChecklist"); + switch (result) { case AUTH_ACL_CANNOT_AUTHENTICATE: debugs(28, 4, "aclMatchAcl: returning 0 user authenticated but not authorised."); @@ -237,12 +254,16 @@ ACLChecklist::checkCallback(allow_t answer) if (auth_user_request) { /* the checklist lock */ - auth_user_request->unlock(); + AUTHUSERREQUESTUNLOCK(auth_user_request, "ACLChecklist"); /* it might have been connection based */ assert(conn() != NULL); - conn()->auth_user_request = NULL; + /* + * DPW 2007-05-08 + * yuck, this make me uncomfortable. why do this here? + * ConnStateData will do its own unlocking. + */ + AUTHUSERREQUESTUNLOCK(conn()->auth_user_request, "conn via ACLChecklist"); conn()->auth_type = AUTH_BROKEN; - auth_user_request = NULL; } callback_ = callback; @@ -335,11 +356,11 @@ ACLChecklist::operator delete (void *address) ACLChecklist::ACLChecklist() : accessList (NULL), my_port (0), request (NULL), reply (NULL), - auth_user_request (NULL) + auth_user_request (NULL), #if SQUID_SNMP - ,snmp_community(NULL) + snmp_community(NULL), #endif - , callback (NULL), + callback (NULL), callback_data (NULL), extacl_entry (NULL), conn_(NULL), @@ -372,6 +393,13 @@ ACLChecklist::~ACLChecklist() HTTPMSGUNLOCK(reply); + /* + * DPW 2007-05-08 + * If this fails, then we'll need a backup UNLOCK call in the + * destructor. + */ + assert(auth_user_request == NULL); + conn_ = NULL; cbdataReferenceDone(accessList); @@ -553,8 +581,6 @@ aclChecklistCreate(const acl_access * A, HttpRequest * request, const char *iden #endif - checklist->auth_user_request = NULL; - return checklist; } diff --git a/src/ACLProxyAuth.cc b/src/ACLProxyAuth.cc index 4f7f1d54ad..b86aac7444 100644 --- a/src/ACLProxyAuth.cc +++ b/src/ACLProxyAuth.cc @@ -160,14 +160,12 @@ ProxyAuthLookup::LookupDone(void *data, char *result) /* credentials could not be checked either way * restart the whole process */ /* OR the connection was closed, there's no way to continue */ - checklist->auth_user_request->unlock(); + AUTHUSERREQUESTUNLOCK(checklist->auth_user_request, "ProxyAuthLookup"); if (checklist->conn() != NULL) { - checklist->conn()->auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(checklist->conn()->auth_user_request, "conn via ProxyAuthLookup"); // DPW discomfort checklist->conn()->auth_type = AUTH_BROKEN; } - - checklist->auth_user_request = NULL; } checklist->asyncInProgress(false); @@ -225,14 +223,8 @@ void ACLProxyAuth::checkAuthForCaching(ACLChecklist *checklist)const { /* for completeness */ - - checklist->auth_user_request->lock() - - ; /* consistent parameters ? */ assert(authenticateUserAuthenticated(checklist->auth_user_request)); - /* this check completed */ - checklist->auth_user_request->unlock(); } diff --git a/src/AuthConfig.cc b/src/AuthConfig.cc index 74a0b2c3f2..0f2e043853 100644 --- a/src/AuthConfig.cc +++ b/src/AuthConfig.cc @@ -1,6 +1,6 @@ /* - * $Id: AuthConfig.cc,v 1.3 2007/04/28 22:26:37 hno Exp $ + * $Id: AuthConfig.cc,v 1.4 2007/05/09 09:07:38 wessels Exp $ * * DEBUG: section 29 Authenticator * AUTHOR: Robert Collins @@ -55,15 +55,13 @@ AuthConfig::CreateAuthUser(const char *proxy_auth) return NULL; } - assert (config != NULL); - AuthUserRequest *result = config->decode (proxy_auth); - /* and lock for the callers instance */ - - if (result != NULL) - result->lock() - ; + /* + * DPW 2007-05-08 + * Do not lock the AuthUserRequest on the caller's behalf. + * Callers must manage their own locks. + */ return result; } diff --git a/src/AuthUserRequest.cc b/src/AuthUserRequest.cc index 299d6e6eff..8c30b259c1 100644 --- a/src/AuthUserRequest.cc +++ b/src/AuthUserRequest.cc @@ -1,6 +1,6 @@ /* - * $Id: AuthUserRequest.cc,v 1.15 2007/05/09 07:36:24 wessels Exp $ + * $Id: AuthUserRequest.cc,v 1.16 2007/05/09 09:07:38 wessels Exp $ * * DO NOT MODIFY NEXT 2 LINES: * arch-tag: 6803fde1-d5a2-4c29-9034-1c0c9f650eb4 @@ -324,6 +324,8 @@ authTryGetUser (AuthUserRequest **auth_user_request, ConnStateData::Pointer & co * callback mechanism like the acl testing routines that will send a 40[1|7] to * the client when rv==AUTH_ACL_CHALLENGE, and will communicate with * the authenticateStart routine for rv==AUTH_ACL_HELPER + * + * Caller is responsible for locking and unlocking their *auth_user_request! */ auth_acl_t @@ -350,19 +352,10 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ if (conn != NULL) { conn->auth_type = AUTH_UNKNOWN; - - if (conn->auth_user_request) - conn->auth_user_request->unlock(); - - conn->auth_user_request = NULL; - } - - if (*auth_user_request) { - /* unlock the ACL lock */ - (*auth_user_request)->unlock(); - auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn"); } + *auth_user_request = NULL; return AUTH_ACL_CHALLENGE; } @@ -389,10 +382,7 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ * authenticateAuthenticate */ assert(*auth_user_request == NULL); - /* unlock the conn lock on the auth_user_request */ - conn->auth_user_request->unlock(); - /* mark the conn as non-authed. */ - conn->auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn"); /* Set the connection auth type */ conn->auth_type = AUTH_UNKNOWN; } @@ -414,8 +404,7 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ "' to '" << proxy_auth << "' (client " << inet_ntoa(src_addr) << ")"); - conn->auth_user_request->unlock(); - conn->auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn"); conn->auth_type = AUTH_UNKNOWN; } } @@ -425,8 +414,8 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ /* beginning of a new request check */ debugs(29, 4, "authenticateAuthenticate: no connection authentication type"); - if (!authenticateValidateUser(*auth_user_request = - AuthConfig::CreateAuthUser(proxy_auth))) { + *auth_user_request = AuthConfig::CreateAuthUser(proxy_auth); + if (!authenticateValidateUser(*auth_user_request)) { if (*auth_user_request == NULL) return AUTH_ACL_CHALLENGE; @@ -434,40 +423,21 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ * the user */ if ((*auth_user_request)->username()) { - /* lock the user for the request structure link */ - - (*auth_user_request)->lock() - - ; request->auth_user_request = *auth_user_request; + AUTHUSERREQUESTLOCK(request->auth_user_request, "request"); } - /* unlock the ACL reference granted by ...createAuthUser. */ - (*auth_user_request)->unlock(); - *auth_user_request = NULL; - return AUTH_ACL_CHALLENGE; } /* the user_request comes prelocked for the caller to createAuthUser (us) */ } else if (request->auth_user_request) { *auth_user_request = request->auth_user_request; - /* lock the user request for this ACL processing */ - - (*auth_user_request)->lock() - - ; } else { assert (conn != NULL); - - if (conn->auth_user_request != NULL) { + if (conn->auth_user_request) { *auth_user_request = conn->auth_user_request; - /* lock the user request for this ACL processing */ - - (*auth_user_request)->lock() - - ; } else { /* failed connection based authentication */ debugs(29, 4, "authenticateAuthenticate: Auth user request " << @@ -475,7 +445,6 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ conn->auth_user_request << " conn type " << conn->auth_type << " authentication failed."); - (*auth_user_request)->unlock(); *auth_user_request = NULL; return AUTH_ACL_CHALLENGE; } @@ -492,22 +461,16 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ case 1: - if (!request->auth_user_request) { - - (*auth_user_request)->lock() - - ; + if (NULL == request->auth_user_request) { request->auth_user_request = *auth_user_request; + AUTHUSERREQUESTLOCK(request->auth_user_request, "request"); } /* fallthrough to -2 */ case -2: - /* this ACL check is finished. Unlock. */ - (*auth_user_request)->unlock(); - + /* this ACL check is finished. */ *auth_user_request = NULL; - return AUTH_ACL_CHALLENGE; case -1: @@ -522,41 +485,24 @@ AuthUserRequest::authenticate(AuthUserRequest ** auth_user_request, http_hdr_typ if (!authenticateUserAuthenticated(*auth_user_request)) { if ((*auth_user_request)->username()) { if (!request->auth_user_request) { - /* lock the user for the request structure link */ - - (*auth_user_request)->lock() - - ; request->auth_user_request = *auth_user_request; + AUTHUSERREQUESTLOCK(request->auth_user_request, "request"); } } - /* this ACL check is finished. Unlock. */ - (*auth_user_request)->unlock(); - *auth_user_request = NULL; - return AUTH_ACL_CHALLENGE; } } /* copy username to request for logging on client-side */ /* the credentials are correct at this point */ - if (!request->auth_user_request) - { - /* lock the user for the request structure link */ - - (*auth_user_request)->lock() - - ; + if (NULL == request->auth_user_request) { request->auth_user_request = *auth_user_request; - + AUTHUSERREQUESTLOCK(request->auth_user_request, "request"); authenticateAuthUserRequestSetIp(*auth_user_request, src_addr); } - /* Unlock the request - we've authenticated it */ - (*auth_user_request)->unlock(); - return AUTH_AUTHENTICATED; } @@ -575,7 +521,7 @@ AuthUserRequest::tryToAuthenticateAndSetAuthUser(AuthUserRequest ** auth_user_re if (!request->auth_user_request && t->lastReply == AUTH_AUTHENTICATED) { request->auth_user_request = t; - t->lock(); + AUTHUSERREQUESTLOCK(request->auth_user_request, "request"); } return t->lastReply; } @@ -688,20 +634,20 @@ authenticateAddTrailer(HttpReply * rep, AuthUserRequest * auth_user_request, Htt void -AuthUserRequest::lock() +AuthUserRequest::_lock() { - debugs(29, 9, "AuthUserRequest::lock: auth_user request '" << this << "' (" << references << " references)."); assert(this); + debugs(29, 9, "AuthUserRequest::lock: auth_user request '" << this << " " << references << "->" << references+1); ++references; } void -AuthUserRequest::unlock() +AuthUserRequest::_unlock() { - debugs(29, 9, "AuthUserRequest::unlock: auth_user request '" << this << "' (" << references << " references) ."); assert(this != NULL); if (references > 0) { + debugs(29, 9, "AuthUserRequest::unlock: auth_user request '" << this << " " << references << "->" << references-1); --references; } else { debugs(29, 1, "Attempt to lower Auth User request " << this << " refcount below 0!"); diff --git a/src/AuthUserRequest.h b/src/AuthUserRequest.h index 1a3be46155..ff0534f223 100644 --- a/src/AuthUserRequest.h +++ b/src/AuthUserRequest.h @@ -1,6 +1,6 @@ /* - * $Id: AuthUserRequest.h,v 1.5 2007/05/09 07:36:24 wessels Exp $ + * $Id: AuthUserRequest.h,v 1.6 2007/05/09 09:07:38 wessels Exp $ * * DO NOT MODIFY NEXT 2 LINES: * arch-tag: 674533af-8b21-4641-b71a-74c4639072a0 @@ -94,11 +94,8 @@ public: char const * getDenyMessage (); size_t refCount() const; - - void lock () - - ; - void unlock (); + void _lock (); // please use AUTHUSERREQUESTLOCK() + void _unlock (); // please use AUTHUSERREQUESTUNLOCK() char const *username() const; @@ -137,4 +134,12 @@ extern int authenticateDirection(AuthUserRequest *); extern int authenticateUserAuthenticated(AuthUserRequest *); extern int authenticateValidateUser(AuthUserRequest *); +#if 0 +#define AUTHUSERREQUESTUNLOCK(a,b) if(a){(a)->_unlock();debugs(0,0,HERE << "auth_user_request " << a << " was unlocked for " << b); (a)=NULL;} +#define AUTHUSERREQUESTLOCK(a,b) { (a)->_lock(); debugs(0,0,HERE << "auth_user_request " << a << " was locked for " << b); } +#endif +#define AUTHUSERREQUESTUNLOCK(a,b) if(a){(a)->_unlock();(a)=NULL;} +#define AUTHUSERREQUESTLOCK(a,b) (a)->_lock() + + #endif /* SQUID_AUTHUSERREQUEST_H */ diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index f880852464..74c0eac8f2 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -1,6 +1,6 @@ /* - * $Id: HttpRequest.cc,v 1.73 2007/04/28 22:26:37 hno Exp $ + * $Id: HttpRequest.cc,v 1.74 2007/05/09 09:07:38 wessels Exp $ * * DEBUG: section 73 HTTP Request * AUTHOR: Duane Wessels @@ -106,10 +106,7 @@ HttpRequest::clean() // points to a pipe that is owned and initiated by another object. body_pipe = NULL; - if (auth_user_request) { - auth_user_request->unlock(); - auth_user_request = NULL; - } + AUTHUSERREQUESTUNLOCK(auth_user_request, "request"); safe_free(canonical); diff --git a/src/auth/digest/auth_digest.cc b/src/auth/digest/auth_digest.cc index 06a29fb22c..0553f03b43 100644 --- a/src/auth/digest/auth_digest.cc +++ b/src/auth/digest/auth_digest.cc @@ -1,6 +1,6 @@ /* - * $Id: auth_digest.cc,v 1.54 2007/05/09 07:36:28 wessels Exp $ + * $Id: auth_digest.cc,v 1.55 2007/05/09 09:07:39 wessels Exp $ * * DEBUG: section 29 Authenticator * AUTHOR: Robert Collins @@ -858,9 +858,7 @@ authenticateDigestHandleReply(void *data, char *reply) replyData->handler(cbdata, NULL); //we know replyData->auth_user_request != NULL, or we'd have asserted - replyData->auth_user_request->unlock(); - - replyData->auth_user_request=NULL; + AUTHUSERREQUESTUNLOCK(replyData->auth_user_request, "replyData"); cbdataFree(replyData); } @@ -1372,11 +1370,7 @@ AuthDigestUserRequest::module_start(RH * handler, void *data) r->handler = handler; r->data = cbdataReference(data); r->auth_user_request = this; - - lock() - - ; - + AUTHUSERREQUESTLOCK(r->auth_user_request, "r"); snprintf(buf, 8192, "\"%s\":\"%s\"\n", digest_user->username(), realm); helperSubmit(digestauthenticators, buf, authenticateDigestHandleReply, r); diff --git a/src/auth/negotiate/auth_negotiate.cc b/src/auth/negotiate/auth_negotiate.cc index 6f41f290d4..8865b36e0c 100644 --- a/src/auth/negotiate/auth_negotiate.cc +++ b/src/auth/negotiate/auth_negotiate.cc @@ -1,6 +1,6 @@ /* - * $Id: auth_negotiate.cc,v 1.17 2007/05/09 08:07:23 wessels Exp $ + * $Id: auth_negotiate.cc,v 1.18 2007/05/09 09:07:40 wessels Exp $ * * DEBUG: section 29 Negotiate Authenticator * AUTHOR: Robert Collins, Henrik Nordstrom, Francesco Chemolli @@ -57,6 +57,7 @@ authenticateNegotiateReleaseServer(AuthUserRequest * auth_user_request); static void authenticateStateFree(authenticateStateData * r) { + AUTHUSERREQUESTUNLOCK(r->auth_user_request, "r"); cbdataFree(r); } @@ -558,11 +559,7 @@ AuthNegotiateUserRequest::module_start(RH * handler, void *data) r = cbdataAlloc(authenticateStateData); r->handler = handler; r->data = cbdataReference(data); - r->auth_user_request = this; - - lock() - - ; + AUTHUSERREQUESTLOCK(r->auth_user_request, "r"); if (auth_state == AUTHENTICATE_STATE_INITIAL) { snprintf(buf, 8192, "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here? } else { @@ -593,13 +590,13 @@ authenticateNegotiateReleaseServer(AuthUserRequest * auth_user_request) /* clear any connection related authentication details */ void -AuthNegotiateUserRequest::onConnectionClose(ConnStateData *connection) +AuthNegotiateUserRequest::onConnectionClose(ConnStateData *conn) { - assert(connection != NULL); + assert(conn != NULL); - debugs(29, 8, "AuthNegotiateUserRequest::onConnectionClose: closing connection '" << connection << "' (this is '" << this << "')"); + debugs(29, 8, "AuthNegotiateUserRequest::onConnectionClose: closing connection '" << conn << "' (this is '" << this << "')"); - if (connection->auth_user_request == NULL) { + if (conn->auth_user_request == NULL) { debugs(29, 8, "AuthNegotiateUserRequest::onConnectionClose: no auth_user_request"); return; } @@ -608,15 +605,8 @@ AuthNegotiateUserRequest::onConnectionClose(ConnStateData *connection) authenticateNegotiateReleaseServer(this); /* unlock the connection based lock */ - debugs(29, 9, "AuthNegotiateUserRequest::onConnectionClose: Unlocking auth_user from the connection '" << connection << "'."); - - /* This still breaks the abstraction, but is at least read only now. - * If needed, this could be ignored, as the conn deletion will also unlock - * the auth user request. - */ - unlock(); - - connection->auth_user_request = NULL; + debugs(29, 9, "AuthNegotiateUserRequest::onConnectionClose: Unlocking auth_user from the connection '" << conn << "'."); + AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn"); } /* @@ -709,12 +699,9 @@ AuthNegotiateUserRequest::authenticate(HttpRequest * request, ConnStateData::Poi safe_free(client_blob); client_blob=xstrdup(blob); conn->auth_type = AUTH_NEGOTIATE; + assert(conn->auth_user_request == NULL); conn->auth_user_request = this; - conn = conn; - - lock() - - ; + AUTHUSERREQUESTLOCK(conn->auth_user_request, "conn"); return; break; diff --git a/src/auth/ntlm/auth_ntlm.cc b/src/auth/ntlm/auth_ntlm.cc index 628c5a673d..9e7945d1e4 100644 --- a/src/auth/ntlm/auth_ntlm.cc +++ b/src/auth/ntlm/auth_ntlm.cc @@ -1,6 +1,6 @@ /* - * $Id: auth_ntlm.cc,v 1.67 2007/05/09 08:14:09 wessels Exp $ + * $Id: auth_ntlm.cc,v 1.68 2007/05/09 09:07:43 wessels Exp $ * * DEBUG: section 29 NTLM Authenticator * AUTHOR: Robert Collins, Henrik Nordstrom, Francesco Chemolli @@ -57,6 +57,7 @@ authenticateNTLMReleaseServer(AuthUserRequest * auth_user_request); static void authenticateStateFree(authenticateStateData * r) { + AUTHUSERREQUESTUNLOCK(r->auth_user_request, "r"); cbdataFree(r); } @@ -390,8 +391,6 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply) ntlm_user->username(blob); auth_user_request->denyMessage("Login successful"); safe_free(ntlm_request->server_blob); - authenticateNTLMReleaseServer(ntlm_request); - ntlm_request->auth_state = AUTHENTICATE_STATE_DONE; result = S_HELPER_RELEASE; debugs(29, 4, "authenticateNTLMHandleReply: Successfully validated user via NTLM. Username '" << blob << "'"); @@ -488,10 +487,8 @@ AuthNTLMUserRequest::module_start(RH * handler, void *data) r->handler = handler; r->data = cbdataReference(data); r->auth_user_request = this; + AUTHUSERREQUESTLOCK(r->auth_user_request, "r"); - lock() - - ; if (auth_state == AUTHENTICATE_STATE_INITIAL) { snprintf(buf, 8192, "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here? } else { @@ -516,19 +513,23 @@ authenticateNTLMReleaseServer(AuthUserRequest * auth_user_request) * Let's see what happens, might segfault in helperStatefulReleaseServer * if it does. I leave it like this not to cover possibly problematic * code-paths. Kinkie */ - helperStatefulReleaseServer(ntlm_request->authserver); - ntlm_request->authserver = NULL; + /* DPW 2007-05-07 + * yes, it is possible */ + if (ntlm_request->authserver) { + helperStatefulReleaseServer(ntlm_request->authserver); + ntlm_request->authserver = NULL; + } } /* clear any connection related authentication details */ void -AuthNTLMUserRequest::onConnectionClose(ConnStateData *connection) +AuthNTLMUserRequest::onConnectionClose(ConnStateData *conn) { - assert(connection != NULL); + assert(conn != NULL); - debugs(29, 8, "AuthNTLMUserRequest::onConnectionClose: closing connection '" << connection << "' (this is '" << this << "')"); + debugs(29, 8, "AuthNTLMUserRequest::onConnectionClose: closing connection '" << conn << "' (this is '" << this << "')"); - if (connection->auth_user_request == NULL) { + if (conn->auth_user_request == NULL) { debugs(29, 8, "AuthNTLMUserRequest::onConnectionClose: no auth_user_request"); return; } @@ -537,15 +538,9 @@ AuthNTLMUserRequest::onConnectionClose(ConnStateData *connection) authenticateNTLMReleaseServer(this); /* unlock the connection based lock */ - debugs(29, 9, "AuthNTLMUserRequest::onConnectionClose: Unlocking auth_user from the connection '" << connection << "'."); + debugs(29, 9, "AuthNTLMUserRequest::onConnectionClose: Unlocking auth_user from the connection '" << conn << "'."); - /* This still breaks the abstraction, but is at least read only now. - * If needed, this could be ignored, as the conn deletion will also unlock - * the auth user request. - */ - unlock(); - - connection->auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn"); } /* @@ -638,12 +633,9 @@ AuthNTLMUserRequest::authenticate(HttpRequest * request, ConnStateData::Pointer safe_free(client_blob); client_blob=xstrdup(blob); conn->auth_type = AUTH_NTLM; + assert(conn->auth_user_request == NULL); conn->auth_user_request = this; - conn = conn; - - lock() - - ; + AUTHUSERREQUESTLOCK(conn->auth_user_request, "conn"); return; break; diff --git a/src/client_side.cc b/src/client_side.cc index 8ff53e28d4..4396227802 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side.cc,v 1.752 2007/04/30 16:56:09 wessels Exp $ + * $Id: client_side.cc,v 1.753 2007/05/09 09:07:38 wessels Exp $ * * DEBUG: section 33 Client-side Routines * AUTHOR: Duane Wessels @@ -467,9 +467,7 @@ clientPrepareLogWithRequestDetails(HttpRequest * request, AccessLogEntry * aLogE aLogEntry->cache.authuser = xstrdup(request->auth_user_request->username()); - request->auth_user_request->unlock(); - - request->auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(request->auth_user_request, "request via clientPrepareLogWithRequestDetails"); } } @@ -625,10 +623,7 @@ ConnStateData::~ConnStateData() if (isOpen()) close(); - if (auth_user_request) - auth_user_request->unlock(); - - auth_user_request = NULL; + AUTHUSERREQUESTUNLOCK(auth_user_request, "~conn"); cbdataReferenceDone(port); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 38dc646c5d..c616f4292c 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side_reply.cc,v 1.125 2007/05/09 07:36:24 wessels Exp $ + * $Id: client_side_reply.cc,v 1.126 2007/05/09 09:07:38 wessels Exp $ * * DEBUG: section 88 Client-side Reply Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -91,7 +91,6 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : http void clientReplyContext::setReplyToError( err_type err, http_status status, method_t method, char const *uri, - struct IN_ADDR *addr, HttpRequest * failedrequest, char *unparsedrequest, AuthUserRequest * auth_user_request) { @@ -112,10 +111,7 @@ clientReplyContext::setReplyToError( if (auth_user_request) { errstate->auth_user_request = auth_user_request; - - errstate->auth_user_request->lock() - - ; + AUTHUSERREQUESTLOCK(errstate->auth_user_request, "errstate"); } assert(errstate->callback_data == NULL); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index f50f6961f2..932e474ba3 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.cc,v 1.84 2007/05/08 16:46:37 rousskov Exp $ + * $Id: client_side_request.cc,v 1.85 2007/05/09 09:07:39 wessels Exp $ * * DEBUG: section 85 Client-side Request Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -824,10 +824,7 @@ ClientRequestContext::clientRedirectDone(char *result) if (old_request->auth_user_request) { new_request->auth_user_request = old_request->auth_user_request; - - new_request->auth_user_request->lock() - - ; + AUTHUSERREQUESTLOCK(new_request->auth_user_request, "new request"); } if (old_request->body_pipe != NULL) { diff --git a/src/errorpage.cc b/src/errorpage.cc index 571bed8cae..5bd14c4431 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1,6 +1,6 @@ /* - * $Id: errorpage.cc,v 1.224 2007/04/30 16:56:09 wessels Exp $ + * $Id: errorpage.cc,v 1.225 2007/05/09 09:07:39 wessels Exp $ * * DEBUG: section 4 Error Generation * AUTHOR: Duane Wessels @@ -482,14 +482,8 @@ errorStateFree(ErrorState * err) wordlistDestroy(&err->ftp.server_msg); safe_free(err->ftp.request); safe_free(err->ftp.reply); - - if (err->auth_user_request) - err->auth_user_request->unlock(); - - err->auth_user_request = NULL; - + AUTHUSERREQUESTUNLOCK(err->auth_user_request, "errstate"); safe_free(err->err_msg); - cbdataFree(err); } diff --git a/src/helper.cc b/src/helper.cc index fa4f450de7..99d961f999 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -1,6 +1,6 @@ /* - * $Id: helper.cc,v 1.83 2007/05/07 18:38:40 wessels Exp $ + * $Id: helper.cc,v 1.84 2007/05/09 09:07:39 wessels Exp $ * * DEBUG: section 84 Helper process maintenance * AUTHOR: Harvest Derived? @@ -171,29 +171,25 @@ helperOpenServers(helper * hlp) helperKickQueue(hlp); } +/* + * DPW 2007-05-08 + * + * helperStatefulOpenServers: create the stateful child helper processes + */ void helperStatefulOpenServers(statefulhelper * hlp) { - char *s; - char *progname; char *shortname; - char *procname; const char *args[HELPER_MAX_ARGS]; char fd_note_buf[FD_DESC_SZ]; - helper_stateful_server *srv; int nargs = 0; - int k; - pid_t pid; - int rfd; - int wfd; - void * hIpc; - wordlist *w; if (hlp->cmdline == NULL) return; - progname = hlp->cmdline->key; + char *progname = hlp->cmdline->key; + char *s; if ((s = strrchr(progname, '/'))) shortname = xstrdup(s + 1); else @@ -201,23 +197,25 @@ helperStatefulOpenServers(statefulhelper * hlp) debugs(84, 1, "helperStatefulOpenServers: Starting " << hlp->n_to_start << " '" << shortname << "' processes"); - procname = (char *)xmalloc(strlen(shortname) + 3); + char *procname = (char *)xmalloc(strlen(shortname) + 3); snprintf(procname, strlen(shortname) + 3, "(%s)", shortname); args[nargs++] = procname; - for (w = hlp->cmdline->next; w && nargs < HELPER_MAX_ARGS; w = w->next) + for (wordlist *w = hlp->cmdline->next; w && nargs < HELPER_MAX_ARGS; w = w->next) args[nargs++] = w->key; args[nargs++] = NULL; assert(nargs <= HELPER_MAX_ARGS); - for (k = 0; k < hlp->n_to_start; k++) { + for (int k = 0; k < hlp->n_to_start; k++) { getCurrentTime(); - rfd = wfd = -1; - pid = ipcCreate(hlp->ipc_type, + int rfd = -1; + int wfd = -1; + void * hIpc; + pid_t pid = ipcCreate(hlp->ipc_type, progname, args, shortname, @@ -233,7 +231,7 @@ helperStatefulOpenServers(statefulhelper * hlp) hlp->n_running++; hlp->n_active++; CBDATA_INIT_TYPE(helper_stateful_server); - srv = cbdataAlloc(helper_stateful_server); + helper_stateful_server *srv = cbdataAlloc(helper_stateful_server); srv->hIpc = hIpc; srv->pid = pid; srv->flags.reserved = S_HELPER_FREE; @@ -319,7 +317,6 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, v } helper_stateful_request *r = new helper_stateful_request; - helper_stateful_server *srv; r->callback = callback; r->data = cbdataReference(data); @@ -357,6 +354,7 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, v StatefulServerEnqueue(lastserver, r); } } else { + helper_stateful_server *srv; if ((srv = StatefulGetFirstAvailable(hlp))) { helperStatefulDispatch(srv, r); } else @@ -366,38 +364,36 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, v debugs(84, 9, "helperStatefulSubmit: placeholder: '" << r->placeholder << "', buf '" << buf << "'."); } +/* + * helperStatefulDefer + * + * find and add a deferred request to a helper + */ helper_stateful_server * helperStatefulDefer(statefulhelper * hlp) -/* find and add a deferred request to a server */ { - if (hlp == NULL) - { + if (hlp == NULL) { debugs(84, 3, "helperStatefulDefer: hlp == NULL"); return NULL; } - dlink_node *n; - helper_stateful_server *srv = NULL, *rv = NULL; - - debugs(84, 5, "helperStatefulDefer: Running servers " << hlp->n_running << "."); + debugs(84, 5, "helperStatefulDefer: Running servers " << hlp->n_running); - if (hlp->n_running == 0) - { + if (hlp->n_running == 0) { debugs(84, 1, "helperStatefulDefer: No running servers!. "); return NULL; } - rv = srv = StatefulGetFirstAvailable(hlp); + helper_stateful_server *rv = StatefulGetFirstAvailable(hlp); - if (rv == NULL) - { + if (rv == NULL) { /* * all currently busy; loop through servers and find server * with the shortest queue */ - for (n = hlp->servers.head; n != NULL; n = n->next) { - srv = (helper_stateful_server *)n->data; + for (dlink_node *n = hlp->servers.head; n != NULL; n = n->next) { + helper_stateful_server *srv = (helper_stateful_server *)n->data; if (srv->flags.reserved == S_HELPER_RESERVED) continue; @@ -416,8 +412,7 @@ helperStatefulDefer(statefulhelper * hlp) } } - if (rv == NULL) - { + if (rv == NULL) { debugs(84, 1, "helperStatefulDefer: None available."); return NULL; } @@ -449,11 +444,9 @@ helperStatefulReset(helper_stateful_server * srv) */ { statefulhelper *hlp = srv->parent; - helper_stateful_request *r; - r = srv->request; + helper_stateful_request *r = srv->request; - if (r != NULL) - { + if (r != NULL) { /* reset attempt DURING an outstaning request */ debugs(84, 1, "helperStatefulReset: RESET During request " << hlp->id_name << " "); srv->flags.busy = 0; @@ -464,12 +457,10 @@ helperStatefulReset(helper_stateful_server * srv) srv->flags.busy = 0; - if (srv->queue.head) - { + if (srv->queue.head) { srv->flags.reserved = S_HELPER_DEFERRED; helperStatefulServerKickQueue(srv); - } else - { + } else { srv->flags.reserved = S_HELPER_FREE; if ((srv->parent->OnEmptyQueue != NULL) && (srv->data)) @@ -479,25 +470,43 @@ helperStatefulReset(helper_stateful_server * srv) } } +/* + * DPW 2007-05-08 + * + * helperStatefulReleaseServer tells the helper that whoever was + * using it no longer needs its services. + * + * If the state is S_HELPER_DEFERRED, decrease the deferred count. + * If the count goes to zero, then it can become S_HELPER_FREE. + * + * If the state is S_HELPER_RESERVED, then it should always + * become S_HELPER_FREE. + */ void helperStatefulReleaseServer(helper_stateful_server * srv) -/*decrease the number of 'waiting' clients that set the helper to be DEFERRED */ { + debugs(84, 3, HERE << "srv-" << srv->index << " flags.reserved = " << srv->flags.reserved); + if (srv->flags.reserved = S_HELPER_FREE) + return; + srv->stats.releases++; - if (srv->flags.reserved == S_HELPER_DEFERRED) - { + if (srv->flags.reserved == S_HELPER_DEFERRED) { assert(srv->deferred_requests); srv->deferred_requests--; + if (srv->deferred_requests) { + debugs(0,0,HERE << "helperStatefulReleaseServer srv->deferred_requests=" << srv->deferred_requests); + return; + } + if (srv->queue.head) { + debugs(0,0,HERE << "helperStatefulReleaseServer srv->queue.head not NULL"); + return; + } } - if (!(srv->deferred_requests) && (srv->flags.reserved == S_HELPER_DEFERRED) && !(srv->queue.head)) - { - srv->flags.reserved = S_HELPER_FREE; - - if ((srv->parent->OnEmptyQueue != NULL) && (srv->data)) - srv->parent->OnEmptyQueue(srv->data); - } + srv->flags.reserved = S_HELPER_FREE; + if (srv->parent->OnEmptyQueue != NULL && srv->data) + srv->parent->OnEmptyQueue(srv->data); } void * @@ -513,7 +522,6 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label) if (!helperStartStats(sentry, hlp, label)) return; - dlink_node *link; storeAppendPrintf(sentry, "program: %s\n", hlp->cmdline->key); storeAppendPrintf(sentry, "number running: %d of %d\n", @@ -537,7 +545,7 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label) "Offset", "Request"); - for (link = hlp->servers.head; link; link = link->next) { + for (dlink_node *link = hlp->servers.head; link; link = link->next) { helper_server *srv = (helper_server*)link->data; double tt = 0.001 * (srv->requests[0] ? tvSubMsec(srv->requests[0]->dispatch_time, current_time) : tvSubMsec(srv->dispatch_time, srv->answer_time)); storeAppendPrintf(sentry, "%7d\t%7d\t%7d\t%11d\t%c%c%c%c\t%7.3f\t%7d\t%s\n", @@ -567,9 +575,6 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label if (!helperStartStats(sentry, hlp, label)) return; - helper_stateful_server *srv; - dlink_node *link; - double tt; storeAppendPrintf(sentry, "program: %s\n", hlp->cmdline->key); storeAppendPrintf(sentry, "number running: %d of %d\n", @@ -594,9 +599,9 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label "Offset", "Request"); - for (link = hlp->servers.head; link; link = link->next) { - srv = (helper_stateful_server *)link->data; - tt = 0.001 * tvSubMsec(srv->dispatch_time, + for (dlink_node *link = hlp->servers.head; link; link = link->next) { + helper_stateful_server *srv = (helper_stateful_server *)link->data; + double tt = 0.001 * tvSubMsec(srv->dispatch_time, srv->flags.busy ? current_time : srv->answer_time); storeAppendPrintf(sentry, "%7d\t%7d\t%7d\t%11d\t%20d\t%c%c%c%c%c\t%7.3f\t%7d\t%s\n", srv->index + 1, @@ -606,7 +611,7 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label (int) srv->deferred_requests, srv->flags.busy ? 'B' : ' ', srv->flags.closing ? 'C' : ' ', - srv->flags.reserved != S_HELPER_FREE ? 'R' : ' ', + srv->flags.reserved == S_HELPER_RESERVED ? 'R' : (srv->flags.reserved == S_HELPER_DEFERRED ? 'D' : ' '), srv->flags.shutdown ? 'S' : ' ', srv->request ? (srv->request->placeholder ? 'P' : ' ') : ' ', tt < 0.0 ? 0.0 : tt, @@ -979,7 +984,7 @@ helperHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, voi return; } - debugs(84, 5, "helperHandleRead: " << len << " bytes from " << hlp->id_name << " #" << srv->index + 1 << "."); + debugs(84, 5, "helperHandleRead: " << len << " bytes from " << hlp->id_name << " #" << srv->index + 1); if (flag != COMM_OK || len <= 0) { if (len < 0) @@ -1089,7 +1094,7 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer } debugs(84, 5, "helperStatefulHandleRead: " << len << " bytes from " << - hlp->id_name << " #" << srv->index + 1 << "."); + hlp->id_name << " #" << srv->index + 1); if (flag != COMM_OK || len <= 0) { @@ -1383,7 +1388,7 @@ StatefulGetFirstAvailable(statefulhelper * hlp) { dlink_node *n; helper_stateful_server *srv = NULL; - debugs(84, 5, "StatefulGetFirstAvailable: Running servers " << hlp->n_running << "."); + debugs(84, 5, "StatefulGetFirstAvailable: Running servers " << hlp->n_running); if (hlp->n_running == 0) return NULL; @@ -1403,6 +1408,7 @@ StatefulGetFirstAvailable(statefulhelper * hlp) if ((hlp->IsAvailable != NULL) && (srv->data != NULL) && !(hlp->IsAvailable(srv->data))) continue; + debugs(84, 5, "StatefulGetFirstAvailable: returning srv-" << srv->index); return srv; }