]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Some AuthUserReuqest polish and bug removal
authorAmos Jeffries <squid3@treenet.co.nz>
Thu, 8 Apr 2010 11:53:16 +0000 (23:53 +1200)
committerAmos Jeffries <squid3@treenet.co.nz>
Thu, 8 Apr 2010 11:53:16 +0000 (23:53 +1200)
* merge multiple authenticate and authenticateChildren members from
  child classes into the parent AuthUserRequest

* severe circular ref-count links between AuthUser object and
  AuthUserRequest. It appears to be unused and causes problems by merely
  existing.

* remove entry from client_side unsettign the auth credentials of a
  request simply because it was being logged.
  The log code woud be better pulling the credentials from the request
  directly when needed instead of cloning the text. RefCount in both request
  and AuthUser holds the info in place until log output is finished.
  But that polish is left for later.

20 files changed:
include/RefCount.h
src/auth/Config.h
src/auth/User.cc
src/auth/User.cci
src/auth/User.h
src/auth/UserRequest.cc
src/auth/basic/auth_basic.cc
src/auth/basic/auth_basic.h
src/auth/basic/basicUserRequest.h
src/auth/digest/auth_digest.cc
src/auth/digest/auth_digest.h
src/auth/digest/digestUserRequest.cc
src/auth/negotiate/auth_negotiate.cc
src/auth/negotiate/auth_negotiate.h
src/auth/negotiate/negotiateUserRequest.cc
src/auth/ntlm/auth_ntlm.cc
src/auth/ntlm/auth_ntlm.h
src/auth/ntlm/ntlmUserRequest.cc
src/client_side.cc
src/tests/testAuth.cc

index 1f9abb9d1c08ef09f9f857ff6a34d3b87a6cecc7..87341efaa03c5290f563c8a16300219f25847e60 100644 (file)
@@ -118,7 +118,7 @@ private:
 struct RefCountable_ {
     RefCountable_():count_(0) {}
 
-    virtual ~RefCountable_() {}
+    virtual ~RefCountable_() { assert(count_ == 0); }
 
     /* Not private, to allow class hierarchies */
     void RefCountReference() const {
index 8c4fd6666962ac3ceb17d8623f439dd5e333b5e6..71855b81d7e835fc408f5f43fb25b6c6df711f84 100644 (file)
 #define SQUID_AUTHCONFIG_H
 
 #include "auth/UserRequest.h"
+#include "HelperChildConfig.h"
 
 class StoreEntry;
 class HttpReply;
 class HttpRequest;
+class wordlist;
 
 /* for http_hdr_type parameters-by-value */
 #include "HttpHeader.h"
@@ -60,7 +62,7 @@ public:
     static AuthUserRequest::Pointer CreateAuthUser(const char *proxy_auth);
 
     static AuthConfig *Find(const char *proxy_auth);
-    AuthConfig() {}
+    AuthConfig() : authenticateChildren(20), authenticate(NULL) {}
 
     virtual ~AuthConfig() {}
 
@@ -119,6 +121,10 @@ public:
     virtual void parse(AuthConfig *, int, char *) = 0;
     /** the http string id */
     virtual const char * type() const = 0;
+
+public:
+    HelperChildConfig authenticateChildren;    
+    wordlist *authenticate;
 };
 
 namespace Auth
index 90f8d70de518e2bf50c8ca3da2b1a89a1d9e4206..cceb296ad751a6b441a833a6dc80dac4a85e8679 100644 (file)
@@ -57,11 +57,14 @@ AuthUser::AuthUser (AuthConfig *aConfig) :
     proxy_auth_list.head = proxy_auth_list.tail = NULL;
     proxy_match_cache.head = proxy_match_cache.tail = NULL;
     ip_list.head = ip_list.tail = NULL;
+#if USER_REQUEST_LOOP_DEAD
     requests.head = requests.tail = NULL;
+#endif
     debugs(29, 5, "AuthUser::AuthUser: Initialised auth_user '" << this << "' with refcount '" << references << "'.");
 }
 
-/* Combine two user structs. ONLY to be called from within a scheme
+/**
+ * Combine two user structs. ONLY to be called from within a scheme
  * module. The scheme module is responsible for ensuring that the
  * two users _can_ be merged without invalidating all the request
  * scheme data. The scheme is also responsible for merging any user
@@ -76,17 +79,21 @@ AuthUser::absorb(AuthUser *from)
      * data
      */
     debugs(29, 5, "authenticateAuthUserMerge auth_user '" << from << "' into auth_user '" << this << "'.");
+#if USER_REQUEST_LOOP_DEAD
     dlink_node *link = from->requests.head;
 
     while (link) {
         AuthUserRequest::Pointer *auth_user_request = static_cast<AuthUserRequest::Pointer*>(link->data);
+        /* add to our list. replace if already present. */
+        addRequest(*auth_user_request);
+        AuthUserRequest::Pointer aur = *(auth_user_request);
+        aur->user(this);
+        /* remove from other list */
         dlink_node *tmplink = link;
         link = link->next;
         dlinkDelete(tmplink, &from->requests);
-        dlinkAddTail(auth_user_request, tmplink, &requests);
-        AuthUserRequest::Pointer aur = *(auth_user_request);
-        aur->user(this);
     }
+#endif /* USER_REQUEST_LOOP_DEAD */
 
     references += from->references;
     from->references = 0;
@@ -95,7 +102,6 @@ AuthUser::absorb(AuthUser *from)
 
 AuthUser::~AuthUser()
 {
-    dlink_node *link, *tmplink;
     debugs(29, 5, "AuthUser::~AuthUser: Freeing auth_user '" << this << "' with refcount '" << references << "'.");
     assert(references == 0);
     /* were they linked in by username ? */
@@ -110,18 +116,21 @@ AuthUser::~AuthUser()
         delete usernamehash;
     }
 
+#if USER_REQUEST_LOOP_DEAD
     /* remove any outstanding requests */
-    link = requests.head;
+    dlink_node *link = requests.head;
 
     while (link) {
         debugs(29, 5, "AuthUser::~AuthUser: removing request entry '" << link->data << "'");
         AuthUserRequest::Pointer *auth_user_request = static_cast<AuthUserRequest::Pointer*>(link->data);
-        tmplink = link;
+        dlink_node *tmplink = link;
         link = link->next;
         dlinkDelete(tmplink, &requests);
+        tmplink->data = NULL;
         dlinkNodeDelete(tmplink);
         *auth_user_request = NULL;
     }
+#endif /* USER_REQUEST_LOOP_DEAD */
 
     /* free cached acl results */
     aclCacheMatchFlush(&proxy_match_cache);
@@ -166,7 +175,6 @@ AuthUser::CachedACLsReset()
         username = auth_user->username();
         /* free cached acl results */
         aclCacheMatchFlush(&auth_user->proxy_match_cache);
-
     }
 
     debugs(29, 3, "AuthUser::CachedACLsReset: Finished.");
index 511a8af746d4de4d8d8f02d1055da32a8b10d22d..91c7988b51efc71af829c0abe2625ed0bd576441 100644 (file)
@@ -57,15 +57,36 @@ AuthUser::username(char const*aString)
     }
 }
 
+#if USER_REQUEST_LOOP_DEAD
 void
 AuthUser::addRequest(AuthUserRequest::Pointer request)
 {
-    /* lock for the request link */
-    /* AYJ: 2009-12-12: WTF? locking the AuthUser because it has a reference to a request? */
-    /*    seems to me like this once-upon-a-time may have been intended to lock the AuthUserRequest */
+    /* lock for the request link (AKA lock because the request links to us) */
     lock();
 
     dlink_node *node = dlinkNodeNew();
 
     dlinkAdd(&request, node, &requests);
 }
+
+void
+AuthUser::doneRequest(AuthUserRequest::Pointer request)
+{
+    /* unlink from the auth_user struct */
+    dlink_node *link = requests.head;
+    AuthUserRequest::Pointer *auth_user_request = NULL;
+
+    while (link) {
+        auth_user_request = static_cast<AuthUserRequest::Pointer*>(link->data);
+        if (*auth_user_request == request) {
+            dlink_node *tmplink = link;
+            link = link->next;
+            dlinkDelete(tmplink, &requests);
+            tmplink->data = NULL;
+            dlinkNodeDelete(tmplink);
+            *auth_user_request = NULL;
+        }
+        link = link->next;
+    }
+}
+#endif /* USER_REQUEST_LOOP_DEAD */
index af31988c16cc83723044783c34b2d80d28b16b5a..ccad09a4a64063d047b626a180cbc58a07969ae1 100644 (file)
@@ -74,9 +74,6 @@ public:
     long expiretime;
     /** how many references are outstanding to this instance */
     size_t references;
-    /** the auth_user_request structures that link to this. Yes it could be a splaytree
-     * but how many requests will a single username have in parallel? */
-    dlink_list requests;
 
     static void cacheInit();
     static void CachedACLsReset();
@@ -85,10 +82,25 @@ public:
     virtual ~AuthUser();
     _SQUID_INLINE_ char const *username() const;
     _SQUID_INLINE_ void username(char const *);
+
+    /* Manage list of IPs using this username */
     void clearIp();
     void removeIp(IpAddress);
     void addIp(IpAddress);
+
+#if USER_REQUEST_LOOP_DEAD
+protected:
+    /* manage list of active authentication requests for this username */
+    /** the auth_user_request structures that link to this. Yes it could be a splaytree
+     * but how many requests will a single username have in parallel? */
+    dlink_list requests;
+
+    /* AYJ: why? do we need this here? it forms the core of a circular refcount. */
+
+public:
     _SQUID_INLINE_ void addRequest(AuthUserRequest::Pointer);
+    _SQUID_INLINE_ void doneRequest(AuthUserRequest::Pointer);
+#endif /* USER_REQUEST_LOOP_DEAD */
 
     void lock();
     void unlock();
@@ -96,10 +108,10 @@ public:
     void addToNameCache();
 
 protected:
-    AuthUser (AuthConfig *);
+    AuthUser(AuthConfig *);
 
 private:
-    static void cacheCleanup (void *unused);
+    static void cacheCleanup(void *unused);
 
     /**
      * DPW 2007-05-08
index 75a281202a02c9f7b67fe2f8acf6190beb1f8c1b..493ae4a0486d343790381bf3236eb9595de67fcf 100644 (file)
@@ -135,32 +135,20 @@ AuthUserRequest::AuthUserRequest():_auth_user(NULL), message(NULL),
 
 AuthUserRequest::~AuthUserRequest()
 {
-    dlink_node *link;
+    assert(RefCountCount()==0);
     debugs(29, 5, "AuthUserRequest::~AuthUserRequest: freeing request " << this);
 
     if (user()) {
+#if USER_REQUEST_LOOP_DEAD
         /* AYJ: something strange: in order to be deleted this object must not be
          * referenced anywhere. Including the AuthUser list of requests.
          * I expect the following loop to NEVER find a pointer to this request object.
          */
+        user()->doneRequest(this);
+#endif /* USER_REQUEST_LOOP_DEAD */
 
-        /* unlink from the auth_user struct */
-        link = user()->requests.head;
-
-        while (link) {
-
-            assert( static_cast<AuthUserRequest::Pointer*>(link->data)->getRaw() != this );
-
-            if (static_cast<AuthUserRequest::Pointer*>(link->data)->getRaw() == this) {
-                dlinkDelete(link, &user()->requests);
-                dlinkNodeDelete(link);
-            }
-            link = link->next;
-        }
-
-        /* unlock the request structure's lock */
+        /* unlock the request structure's lock and release it */
         user()->unlock();
-
         user(NULL);
     }
 
index 550119e38315e76c81bab17be4cfe16af15f2416..d73c952c1371bd8db34eadadcb02830a2e3296b2 100644 (file)
@@ -212,8 +212,6 @@ AuthBasicConfig::dump(StoreEntry * entry, const char *name, AuthConfig * scheme)
 }
 
 AuthBasicConfig::AuthBasicConfig() :
-        authenticateChildren(20),
-        authenticate(NULL),
         credentialsTTL( 2*60*60 ),
         casesensitive(0),
         utf8(0)
@@ -387,6 +385,10 @@ BasicUser::valid() const
     return true;
 }
 
+/**
+ * Generate a duplicate of the bad credentials before clearing the working copy.
+ * apparently for logging, but WTF?!
+ */
 void
 BasicUser::makeLoggingInstance(AuthUserRequest::Pointer auth_user_request)
 {
@@ -401,8 +403,10 @@ BasicUser::makeLoggingInstance(AuthUserRequest::Pointer auth_user_request)
         username(NULL);
         /* set the auth_user type */
         basic_auth->auth_type = AUTH_BROKEN;
+#if USER_REQUEST_LOOP_DEAD
         /* link the request to the user */
         basic_auth->addRequest(auth_user_request);
+#endif /* USER_REQUEST_LOOP_DEAD */
     }
 }
 
@@ -494,7 +498,9 @@ AuthBasicConfig::decode(char const *proxy_auth)
 
     /* link the request to the in-cache user */
     auth_user_request->user(basic_auth);
+#if USER_REQUEST_LOOP_DEAD
     basic_auth->addRequest(auth_user_request);
+#endif /* USER_REQUEST_LOOP_DEAD */
     return auth_user_request;
 }
 
index 29f9dc11f6b821ca4956264dde78f6ef0142958f..e9f12d008902643201cbbcb3d7e6609923435d63 100644 (file)
@@ -67,8 +67,6 @@ MEMPROXY_CLASS_INLINE(BasicUser);
 
 typedef class BasicUser basic_data;
 
-#include "HelperChildConfig.h"
-
 /* configuration runtime data */
 
 class AuthBasicConfig : public AuthConfig
@@ -87,8 +85,6 @@ public:
     virtual void parse(AuthConfig *, int, char *);
     virtual void registerWithCacheManager(void);
     virtual const char * type() const;
-    HelperChildConfig authenticateChildren;
-    wordlist *authenticate;
     char *basicAuthRealm;
     time_t credentialsTTL;
     int casesensitive;
index 7e1975ad25bb1a5b08cc0c5ad985be8949597a8a..fabd599247b1606118fcb4608ef8d2ab880b8f8c 100644 (file)
@@ -16,7 +16,7 @@ public:
     MEMPROXY_CLASS(AuthBasicUserRequest);
 
     AuthBasicUserRequest() {};
-    virtual ~AuthBasicUserRequest() {};
+    virtual ~AuthBasicUserRequest() { assert(RefCountCount()==0); };
 
     virtual int authenticated() const;
     virtual void authenticate(HttpRequest * request, ConnStateData *conn, http_hdr_type type);
index 865a142d999dd012189d8cfcb385ecae1958f829..8a163e500e1ce942cb295ccd5c0c7922fee7a2b9 100644 (file)
@@ -63,7 +63,7 @@ static hash_table *digest_nonce_cache;
 static int authdigest_initialised = 0;
 static MemAllocator *digest_nonce_pool = NULL;
 
-CBDATA_TYPE(DigestAuthenticateStateData);
+// CBDATA_TYPE(DigestAuthenticateStateData);
 
 enum http_digest_attr_type {
     DIGEST_USERNAME,
@@ -676,7 +676,7 @@ AuthDigestConfig::done()
     safe_free(digestAuthRealm);
 }
 
-AuthDigestConfig::AuthDigestConfig() : authenticateChildren(20), authenticate(NULL)
+AuthDigestConfig::AuthDigestConfig()
 {
     /* TODO: move into initialisation list */
     /* 5 minutes */
@@ -828,7 +828,9 @@ authDigestLogUsername(char *username, AuthUserRequest::Pointer auth_user_request
     /* link the request to the user */
     auth_user_request->user(digest_user);
     digest_user->lock();
+#if USER_REQUEST_LOOP_DEAD
     digest_user->addRequest(auth_user_request);
+#endif
     return auth_user_request;
 }
 
@@ -1090,8 +1092,9 @@ AuthDigestConfig::decode(char const *proxy_auth)
 
     digest_user->lock();
     digest_request->user(digest_user);
-
+#if USER_REQUEST_LOOP_DEAD
     digest_user->addRequest(digest_request);
+#endif
 
     debugs(29, 9, "username = '" << digest_user->username() << "'\nrealm = '" <<
            digest_request->realm << "'\nqop = '" << digest_request->qop <<
index 8b021ddb2f459719583ea0ef792ce4eb6e87597a..dfbe62ae6a4f8650f3765e594bd65ea952c860c4 100644 (file)
@@ -74,8 +74,6 @@ extern int authDigestNonceIsValid(digest_nonce_h * nonce, char nc[9]);
 extern const char *authenticateDigestNonceNonceb64(const digest_nonce_h * nonce);
 extern const int authDigestNonceLastRequest(digest_nonce_h * nonce);
 
-#include "HelperChildConfig.h"
-
 /* configuration runtime data */
 
 class AuthDigestConfig : public AuthConfig
@@ -93,9 +91,7 @@ public:
     virtual void parse(AuthConfig *, int, char *);
     virtual void registerWithCacheManager(void);
     virtual const char * type() const;
-    HelperChildConfig authenticateChildren;
     char *digestAuthRealm;
-    wordlist *authenticate;
     time_t nonceGCInterval;
     time_t noncemaxduration;
     unsigned int noncemaxuses;
index de45320b3b6abe13c54fde10dcdbfca31e16430f..e3c06ce53086da9e31dd7faf92e71d4182a0cd1f 100644 (file)
@@ -26,6 +26,8 @@ AuthDigestUserRequest::AuthDigestUserRequest() :
  */
 AuthDigestUserRequest::~AuthDigestUserRequest()
 {
+    assert(RefCountCount()==0);
+
     safe_free(nonceb64);
     safe_free(cnonce);
     safe_free(realm);
index b8ae8ed092e09dac6223e8c2e74210e57073d001..6e9a6ff630231ecd5fdddeb14df31dd2fd489929 100644 (file)
@@ -116,7 +116,7 @@ AuthNegotiateConfig::dump(StoreEntry * entry, const char *name, AuthConfig * sch
 
 }
 
-AuthNegotiateConfig::AuthNegotiateConfig() : authenticateChildren(20), keep_alive(1), authenticate(NULL)
+AuthNegotiateConfig::AuthNegotiateConfig() : keep_alive(1)
 { }
 
 void
@@ -312,7 +312,9 @@ AuthNegotiateConfig::decode(char const *proxy_auth)
 
     auth_user_request->user(newUser);
     auth_user_request->user()->auth_type = AUTH_NEGOTIATE;
+#if USER_REQUEST_LOOP_DEAD
     auth_user_request->user()->addRequest(auth_user_request);
+#endif
 
     /* all we have to do is identify that it's Negotiate - the helper does the rest */
     debugs(29, 9, "AuthNegotiateConfig::decode: Negotiate authentication");
index 22f9b37dad4287dcdbadb568ab3b191b77c57012..d0cdd3103854784946c10e71c3fe91bb74b01bc6 100644 (file)
@@ -35,8 +35,6 @@ public:
 
 MEMPROXY_CLASS_INLINE(NegotiateUser);
 
-#include "HelperChildConfig.h"
-
 extern statefulhelper *negotiateauthenticators;
 
 /* configuration runtime data */
@@ -57,9 +55,7 @@ public:
     virtual void parse(AuthConfig *, int, char *);
     virtual void registerWithCacheManager(void);
     virtual const char * type() const;
-    HelperChildConfig authenticateChildren;
     int keep_alive;
-    wordlist *authenticate;
 };
 
 extern AuthNegotiateConfig negotiateConfig;
index 9935dc8ef0d16f1425ed65e6c1c59441f1abb0c3..dbf4511131ceb96f79a70fab43331c4500d0700f 100644 (file)
@@ -25,6 +25,7 @@ AuthNegotiateUserRequest::AuthNegotiateUserRequest() :
 
 AuthNegotiateUserRequest::~AuthNegotiateUserRequest()
 {
+    assert(RefCountCount()==0);
     safe_free(server_blob);
     safe_free(client_blob);
 
index 36eb0a5b39b32094b4e1a66c13f5e15e25f5c696..dbc1515e29aa437f64701e097d40f87217088198 100644 (file)
@@ -104,7 +104,7 @@ AuthNTLMConfig::dump(StoreEntry * entry, const char *name, AuthConfig * scheme)
 
 }
 
-AuthNTLMConfig::AuthNTLMConfig() : authenticateChildren(20), keep_alive(1), authenticate(NULL)
+AuthNTLMConfig::AuthNTLMConfig() : keep_alive(1)
 { }
 
 void
@@ -283,7 +283,9 @@ AuthNTLMConfig::decode(char const *proxy_auth)
 
     auth_user_request->user(newUser);
     auth_user_request->user()->auth_type = AUTH_NTLM;
+#if USER_REQUEST_LOOP_DEAD
     auth_user_request->user()->addRequest(auth_user_request);
+#endif
 
     /* all we have to do is identify that it's NTLM - the helper does the rest */
     debugs(29, 9, "AuthNTLMConfig::decode: NTLM authentication");
index 58f532d9978ccc02c018af73b7115cecb0641edc..d61eece5e1320e65bc63fe0a76df974ad3e1f967 100644 (file)
@@ -28,8 +28,6 @@ MEMPROXY_CLASS_INLINE(NTLMUser);
 
 typedef class NTLMUser ntlm_user_t;
 
-#include "HelperChildConfig.h"
-
 /* configuration runtime data */
 
 class AuthNTLMConfig : public AuthConfig
@@ -47,9 +45,7 @@ public:
     virtual void parse(AuthConfig *, int, char *);
     virtual void registerWithCacheManager(void);
     virtual const char * type() const;
-    HelperChildConfig authenticateChildren;
     int keep_alive;
-    wordlist *authenticate;
 };
 
 typedef class AuthNTLMConfig auth_ntlm_config;
index 34317dbb7e0254abd2c97a35153a09440129d653..0e41ccf7a8bcbf0c4d476dd8278dd9220ccf6d68 100644 (file)
@@ -19,6 +19,7 @@ AuthNTLMUserRequest::AuthNTLMUserRequest() :
 
 AuthNTLMUserRequest::~AuthNTLMUserRequest()
 {
+    assert(RefCountCount()==0);
     safe_free(server_blob);
     safe_free(client_blob);
 
index 60defc884f84dedf47ec10856ce640e9f88b09c0..ec53114f4f0bd95ffd94f1308713525f432b6ddc 100644 (file)
@@ -504,7 +504,7 @@ prepareLogWithRequestDetails(HttpRequest * request, AccessLogEntry * aLogEntry)
         if (request->auth_user_request->username())
             aLogEntry->cache.authuser = xstrdup(request->auth_user_request->username());
 
-        request->auth_user_request = NULL;
+// WTF??        request->auth_user_request = NULL;
     }
 }
 
index b4ea3563963bf05c602e4ab4266985f6f607bfb4..449cb689c58023de6faf8259ba08c8d4f034f65f 100644 (file)
@@ -202,7 +202,9 @@ testAuthBasicUserRequest::username()
     BasicUser *basic_auth=new BasicUser(AuthConfig::Find("basic"));
     basic_auth->username("John");
     temp->user(basic_auth);
+#if USER_REQUEST_LOOP_DEAD
     basic_auth->addRequest(temp);
+#endif
     CPPUNIT_ASSERT_EQUAL(0, strcmp("John", temp->username()));
 }
 #endif /* HAVE_AUTH_MODULE_BASIC */
@@ -226,7 +228,9 @@ testAuthDigestUserRequest::username()
     DigestUser *duser=new DigestUser(AuthConfig::Find("digest"));
     duser->username("John");
     temp->user(duser);
+#if USER_REQUEST_LOOP_DEAD
     duser->addRequest(temp);
+#endif
     CPPUNIT_ASSERT_EQUAL(0, strcmp("John", temp->username()));
 }
 #endif /* HAVE_AUTH_MODULE_DIGEST */
@@ -250,7 +254,9 @@ testAuthNTLMUserRequest::username()
     NTLMUser *nuser=new NTLMUser(AuthConfig::Find("ntlm"));
     nuser->username("John");
     temp->user(nuser);
+#if USER_REQUEST_LOOP_DEAD
     nuser->addRequest(temp);
+#endif
     CPPUNIT_ASSERT_EQUAL(0, strcmp("John", temp->username()));
 }
 #endif /* HAVE_AUTH_MODULE_NTLM */
@@ -274,7 +280,9 @@ testAuthNegotiateUserRequest::username()
     NegotiateUser *nuser=new NegotiateUser(AuthConfig::Find("negotiate"));
     nuser->username("John");
     temp->user(nuser);
+#if USER_REQUEST_LOOP_DEAD
     nuser->addRequest(temp);
+#endif
     CPPUNIT_ASSERT_EQUAL(0, strcmp("John", temp->username()));
 }