From: Francesco Chemolli Date: Thu, 3 Sep 2015 21:03:49 +0000 (+0200) Subject: Interim: remove proxy_auth_username_cache. X-Git-Tag: SQUID_4_0_1~21^2~27 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d066817ea87ecfdbed17b6ac317752a7af678279;p=thirdparty%2Fsquid.git Interim: remove proxy_auth_username_cache. Attempt fixing cache cleanup and strange iterator behaviors. May crash on shutdown if there are entries in cache. --- diff --git a/src/auth/Gadgets.cc b/src/auth/Gadgets.cc index 4ab485a990..3eddb8f285 100644 --- a/src/auth/Gadgets.cc +++ b/src/auth/Gadgets.cc @@ -69,10 +69,6 @@ authenticateRegisterWithCacheManager(Auth::ConfigVector * config) void authenticateInit(Auth::ConfigVector * config) { - /* Do this first to clear memory and remove dead state on a reconfigure */ - if (proxy_auth_username_cache) - Auth::User::CachedACLsReset(); - /* If we do not have any auth config state to create stop now. */ if (!config) return; @@ -84,9 +80,6 @@ authenticateInit(Auth::ConfigVector * config) schemeCfg->init(schemeCfg); } - if (!proxy_auth_username_cache) - Auth::User::cacheInit(); - authenticateRegisterWithCacheManager(config); } @@ -101,16 +94,9 @@ authenticateRotate(void) void authenticateReset(void) { - debugs(29, 2, HERE << "Reset authentication State."); - - /* free all username cache entries */ - hash_first(proxy_auth_username_cache); - AuthUserHashPointer *usernamehash; - while ((usernamehash = ((AuthUserHashPointer *) hash_next(proxy_auth_username_cache)))) { - debugs(29, 5, HERE << "Clearing entry for user: " << usernamehash->user()->username()); - hash_remove_link(proxy_auth_username_cache, (hash_link *)usernamehash); - delete usernamehash; - } + debugs(29, 2, "Reset authentication State."); + + // username cache is cleared via Runner registry /* schedule shutdown of the helpers */ authenticateRotate(); @@ -119,20 +105,6 @@ authenticateReset(void) Auth::TheConfig.clear(); } -AuthUserHashPointer::AuthUserHashPointer(Auth::User::Pointer anAuth_user): - auth_user(anAuth_user) -{ - key = (void *)anAuth_user->userKey(); - next = NULL; - hash_join(proxy_auth_username_cache, (hash_link *) this); -} - -Auth::User::Pointer -AuthUserHashPointer::user() const -{ - return auth_user; -} - std::vector authenticateCachedUsersList() { diff --git a/src/auth/Gadgets.h b/src/auth/Gadgets.h index 4174f9a083..3a312522b6 100644 --- a/src/auth/Gadgets.h +++ b/src/auth/Gadgets.h @@ -15,32 +15,6 @@ #include "auth/User.h" #include "hash.h" -/** - \ingroup AuthAPI - * - * This is used to link AuthUsers objects into the username cache. - * Because some schemes may link in aliases to a user, - * the link is not part of the AuthUser structure itself. - * - * Code must not hold onto copies of these objects. - * They may exist only so long as the AuthUser being referenced - * is recorded in the cache. Any caller using hash_remove_link - * must then delete the AuthUserHashPointer. - */ -class AuthUserHashPointer : public hash_link -{ - MEMPROXY_CLASS(AuthUserHashPointer); - -public: - AuthUserHashPointer(Auth::User::Pointer); - ~AuthUserHashPointer() { auth_user = NULL; }; - - Auth::User::Pointer user() const; - -private: - Auth::User::Pointer auth_user; -}; - namespace Auth { class Scheme; diff --git a/src/auth/User.cc b/src/auth/User.cc index ad7e801676..305fbbab69 100644 --- a/src/auth/User.cc +++ b/src/auth/User.cc @@ -22,8 +22,6 @@ #include "SquidTime.h" #include "Store.h" -time_t Auth::User::last_discard = 0; - Auth::User::User(Auth::Config *aConfig, const char *aRequestRealm) : auth_type(Auth::AUTH_UNKNOWN), config(aConfig), @@ -139,84 +137,6 @@ Auth::User::~User() auth_type = Auth::AUTH_UNKNOWN; } -void -Auth::User::cacheInit(void) -{ - if (!proxy_auth_username_cache) { - /* First time around, 7921 should be big enough */ - proxy_auth_username_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string); - assert(proxy_auth_username_cache); - eventAdd("User Cache Maintenance", cacheCleanup, NULL, ::Config.authenticateGCInterval, 1); - last_discard = squid_curtime; - } -} - -void -Auth::User::CachedACLsReset() -{ - /* - * This must complete all at once, because we are ensuring correctness. - */ - AuthUserHashPointer *usernamehash; - Auth::User::Pointer auth_user; - debugs(29, 3, HERE << "Flushing the ACL caches for all users."); - hash_first(proxy_auth_username_cache); - - while ((usernamehash = ((AuthUserHashPointer *) hash_next(proxy_auth_username_cache)))) { - auth_user = usernamehash->user(); - /* free cached acl results */ - aclCacheMatchFlush(&auth_user->proxy_match_cache); - } - - //new-style moved to UserNameCache::syncConfig() - - debugs(29, 3, HERE << "Finished."); -} - -void -Auth::User::cacheCleanup(void *) -{ - /* - * We walk the hash by username as that is the unique key we use. - * For big hashs we could consider stepping through the cache, 100/200 - * entries at a time. Lets see how it flys first. - */ - AuthUserHashPointer *usernamehash; - Auth::User::Pointer auth_user; - char const *username = NULL; - debugs(29, 3, HERE << "Cleaning the user cache now"); - debugs(29, 3, HERE << "Current time: " << current_time.tv_sec); - hash_first(proxy_auth_username_cache); - - while ((usernamehash = ((AuthUserHashPointer *) hash_next(proxy_auth_username_cache)))) { - auth_user = usernamehash->user(); - username = auth_user->username(); - - /* if we need to have indedendent expiry clauses, insert a module call - * here */ - debugs(29, 4, HERE << "Cache entry:\n\tType: " << - auth_user->auth_type << "\n\tUsername: " << username << - "\n\texpires: " << - (long int) (auth_user->expiretime + ::Config.authenticateTTL) << - "\n\treferences: " << auth_user->LockCount()); - - if (auth_user->expiretime + ::Config.authenticateTTL <= current_time.tv_sec) { - debugs(29, 5, HERE << "Removing user " << username << " from cache due to timeout."); - - /* Old credentials are always removed. Existing users must hold their own - * Auth::User::Pointer to the credentials. Cache exists only for finding - * and re-using current valid credentials. - */ - hash_remove_link(proxy_auth_username_cache, usernamehash); - delete usernamehash; - } - } - - debugs(29, 3, HERE << "Finished cleaning the user cache."); - eventAdd("User Cache Maintenance", cacheCleanup, NULL, ::Config.authenticateGCInterval, 1); - last_discard = squid_curtime; -} - void Auth::User::clearIp() { diff --git a/src/auth/User.h b/src/auth/User.h index e849b20db7..923a5c9208 100644 --- a/src/auth/User.h +++ b/src/auth/User.h @@ -20,7 +20,6 @@ #include "Notes.h" #include "SBuf.h" -class AuthUserHashPointer; class StoreEntry; namespace Auth @@ -58,8 +57,6 @@ public: NotePairs notes; public: - static void cacheInit(); - static void CachedACLsReset(); static SBuf BuildUserKey(const char *username, const char *realm); void absorb(Auth::User::Pointer from); @@ -107,12 +104,6 @@ protected: User(Auth::Config *, const char *requestRealm); private: - /** - * Garbage Collection for the username cache. - */ - static void cacheCleanup(void *unused); - static time_t last_discard; /// Time of last username cache garbage collection. - /** * DPW 2007-05-08 * The username_ memory will be allocated via diff --git a/src/auth/UserNameCache.cc b/src/auth/UserNameCache.cc index efac3690f4..5aa0677eb8 100644 --- a/src/auth/UserNameCache.cc +++ b/src/auth/UserNameCache.cc @@ -52,13 +52,23 @@ UserNameCache::Cleanup(void *data) UserNameCache *self = static_cast(data); // cache entries with expiretime <= expirationTime are to be evicted const time_t expirationTime = current_time.tv_sec - ::Config.authenticateTTL; + + //XXX for some reason if evicting directly iterators are invalidated + // trying to defer deletion by using a queue + std::list toDelete; const auto end = self->store_.end(); for (auto i = self->store_.begin(); i != end; ++i) { + debugs(29, 2, "considering " << i->first << "(expires in " << + (expirationTime - i->second->expiretime) << " sec)"); if (i->second->expiretime <= expirationTime) { - debugs(29, 2, "evicting " << i->first); - self->store_.erase(i); + debugs(29, 2, "queueing for eviction " << i->first); + toDelete.push_back(i); } } + for (auto i : toDelete) { + debugs(29, 2, "evicting " << i->first); + self->store_.erase(i); //less efficient but safer than iter + } eventAdd(self->cacheCleanupEventName.c_str(), &UserNameCache::Cleanup, self, ::Config.authenticateGCInterval, 1); } diff --git a/src/auth/basic/User.cc b/src/auth/basic/User.cc index f6b8d669e6..8c31744c6b 100644 --- a/src/auth/basic/User.cc +++ b/src/auth/basic/User.cc @@ -9,7 +9,6 @@ #include "squid.h" #include "auth/basic/Config.h" #include "auth/basic/User.h" -#include "auth/Gadgets.h" // for AuthUserHashPointer #include "auth/UserNameCache.h" #include "Debug.h" #include "SquidConfig.h" @@ -92,7 +91,5 @@ Auth::Basic::User::Cache() void Auth::Basic::User::addToNameCache() { - /* AuthUserHashPointer will self-register with the username cache */ - new AuthUserHashPointer(this); //legacy Cache()->insert(this); } diff --git a/src/auth/digest/User.cc b/src/auth/digest/User.cc index 5dd558fc6e..21d833b49c 100644 --- a/src/auth/digest/User.cc +++ b/src/auth/digest/User.cc @@ -9,7 +9,6 @@ #include "squid.h" #include "auth/digest/Config.h" #include "auth/digest/User.h" -#include "auth/Gadgets.h" // for AuthUserHashPointer #include "auth/UserNameCache.h" #include "Debug.h" #include "dlink.h" @@ -84,7 +83,5 @@ Auth::Digest::User::Cache() void Auth::Digest::User::addToNameCache() { - /* AuthUserHashPointer will self-register with the username cache */ - new AuthUserHashPointer(this); //legacy Cache()->insert(this); } diff --git a/src/auth/negotiate/User.cc b/src/auth/negotiate/User.cc index 47bdfb6889..2c9af467f7 100644 --- a/src/auth/negotiate/User.cc +++ b/src/auth/negotiate/User.cc @@ -8,7 +8,6 @@ #include "squid.h" #include "auth/Config.h" -#include "auth/Gadgets.h" // for AuthUserHashPointer #include "auth/negotiate/User.h" #include "auth/UserNameCache.h" #include "Debug.h" @@ -39,7 +38,5 @@ Auth::Negotiate::User::Cache() void Auth::Negotiate::User::addToNameCache() { - /* AuthUserHashPointer will self-register with the username cache */ - new AuthUserHashPointer(this); //legacy Cache()->insert(this); } diff --git a/src/auth/ntlm/User.cc b/src/auth/ntlm/User.cc index 57b23e4d44..6e218b3c20 100644 --- a/src/auth/ntlm/User.cc +++ b/src/auth/ntlm/User.cc @@ -8,7 +8,6 @@ #include "squid.h" #include "auth/Config.h" -#include "auth/Gadgets.h" // for AuthUserHashPointer #include "auth/ntlm/User.h" #include "auth/UserNameCache.h" #include "Debug.h" @@ -39,7 +38,5 @@ Auth::Ntlm::User::Cache() void Auth::Ntlm::User::addToNameCache() { - /* AuthUserHashPointer will self-register with the username cache */ - new AuthUserHashPointer(this); //legacy Cache()->insert(this); } diff --git a/src/globals.h b/src/globals.h index 2f8c7d8af4..b317871acd 100644 --- a/src/globals.h +++ b/src/globals.h @@ -88,7 +88,6 @@ extern int store_swap_low; /* 0 */ extern int store_swap_high; /* 0 */ extern size_t store_pages_max; /* 0 */ extern int64_t store_maxobjsize; /* 0 */ -extern hash_table *proxy_auth_username_cache; /* NULL */ extern int incoming_sockets_accepted; #if _SQUID_WINDOWS_ extern unsigned int WIN32_Socks_initialized; /* 0 */ diff --git a/src/tests/stub_libauth.cc b/src/tests/stub_libauth.cc index 00e9924027..10e22d57e6 100644 --- a/src/tests/stub_libauth.cc +++ b/src/tests/stub_libauth.cc @@ -25,9 +25,6 @@ void authenticateInit(Auth::ConfigVector *) STUB void authenticateRotate(void) STUB void authenticateReset(void) STUB -AuthUserHashPointer::AuthUserHashPointer(Auth::User::Pointer anAuth_user) STUB -Auth::User::Pointer AuthUserHashPointer::user() const STUB_RETVAL(NULL) - #include "auth/Scheme.h" #include std::vector *Auth::Scheme::_Schemes = NULL;