HPCC-16030 Security Manager cache to implement read/write locks

Currently critical sections are used to guard the LDAP cache. This PR
implements the usage of read/write locks, to allow concurrent reads
while no updates are taking place

Signed-off-by: Russ Whitehead <william.whitehead@lexisnexis.com>

system/security/shared/caching.cpp

@@ -54,18 +54,12 @@ CResPermissionsCache::~CResPermissionsCache()
     }   
 }
 
+//called from within a ReadLockBlock
 int CResPermissionsCache::lookup( IArrayOf<ISecResource>& resources, bool* pFound )
 {
     time_t tstamp;
     time(&tstamp);
 
-    int timeout = m_pParentCache->getCacheTimeout();
-    if(timeout == 0 && m_pParentCache->isTransactionalEnabled())
-        timeout = 10; //Transactional timeout is set to 10 seconds for long transactions that might take over 10 seconds.
-    tstamp -= timeout;
-    if (m_tLastCleanup < tstamp)
-        removeStaleEntries(tstamp);
-
     int nresources = resources.ordinality();
     int nFound = 0;
 
@@ -122,6 +116,7 @@ int CResPermissionsCache::lookup( IArrayOf<ISecResource>& resources, bool* pFoun
     return nFound;
 }
 
+//called from within a WriteLockBlock
 void CResPermissionsCache::add( IArrayOf<ISecResource>& resources )
 {
     time_t tstamp;
@@ -173,29 +168,38 @@ void CResPermissionsCache::add( IArrayOf<ISecResource>& resources )
     }
 }
 
+//called from within a WriteLockBlock
 void CResPermissionsCache::removeStaleEntries(time_t tstamp)
 {
-    MapTimeStamp::iterator i; 
-    MapTimeStamp::iterator itL    = m_timestampMap.lower_bound(tstamp);
-    MapTimeStamp::iterator iBegin = m_timestampMap.begin();
-
-    for (i = iBegin; i != itL; i++)
+    int timeout = m_pParentCache->getCacheTimeout();
+    if(timeout == 0 && m_pParentCache->isTransactionalEnabled())
+        timeout = 10; //Transactional timeout is set to 10 seconds for long transactions that might take over 10 seconds.
+    tstamp -= timeout;
+    if (m_tLastCleanup < tstamp)
     {
-        SecCacheKeyEntry& cachekey = (*i).second;
-        MapResAccess::iterator it = m_resAccessMap.find(cachekey);
-        if (it != m_resAccessMap.end())//exists in cache
+        MapTimeStamp::iterator i;
+        MapTimeStamp::iterator itL    = m_timestampMap.lower_bound(tstamp);
+        MapTimeStamp::iterator iBegin = m_timestampMap.begin();
+
+        for (i = iBegin; i != itL; i++)
         {
-            ResPermCacheEntry& entry = (*it).second;
-            if(entry.second)
-                entry.second->Release();
+            SecCacheKeyEntry& cachekey = (*i).second;
+            MapResAccess::iterator it = m_resAccessMap.find(cachekey);
+            if (it != m_resAccessMap.end())//exists in cache
+            {
+                ResPermCacheEntry& entry = (*it).second;
+                if(entry.second)
+                    entry.second->Release();
+            }
+            m_resAccessMap.erase(cachekey);
         }
-        m_resAccessMap.erase(cachekey);
-    }
 
-    m_timestampMap.erase(iBegin, itL);
-    m_tLastCleanup = tstamp;
+        m_timestampMap.erase(iBegin, itL);
+        m_tLastCleanup = tstamp;
+    }
 }
 
+//called from within a WriteLockBlock
 void CResPermissionsCache::remove(SecResourceType rtype, const char* resourcename)
 {
     SecCacheKeyEntry key(resourcename, rtype);
@@ -223,12 +227,27 @@ CPermissionsCache::~CPermissionsCache()
     flush();
 }
 
-int CPermissionsCache::lookup( ISecUser& sec_user, IArrayOf<ISecResource>& resources, 
-                            bool* pFound)
+int CPermissionsCache::lookup( ISecUser& sec_user, IArrayOf<ISecResource>& resources, bool* pFound)
 {
+    time_t tstamp;
+    time(&tstamp);
+
     const char* userId = sec_user.getName();
+
+    //First, clear stale cache entries for this CResPermissionsCache entry
+    {
+        WriteLockBlock writeLock(m_resPermCacheRWLock);
+        MapResPermissionsCache::const_iterator i = m_resPermissionsMap.find( userId );
+        if (i != m_resPermissionsMap.end())
+        {
+            CResPermissionsCache* pResPermissionsCache = (*i).second;
+            pResPermissionsCache->removeStaleEntries(tstamp);
+        }
+    }
+
+    //Lookup all user/resources
     int nFound;
-    CriticalBlock block(m_resPermCacheLock);
+    ReadLockBlock readLock(m_resPermCacheRWLock);
     MapResPermissionsCache::const_iterator i = m_resPermissionsMap.find( userId ); 
     if (i != m_resPermissionsMap.end())
     {
@@ -252,7 +271,7 @@ int CPermissionsCache::lookup( ISecUser& sec_user, IArrayOf<ISecResource>& resou
 void CPermissionsCache::add( ISecUser& sec_user, IArrayOf<ISecResource>& resources )
 {
     const char* user = sec_user.getName();
-    CriticalBlock block(m_resPermCacheLock);
+    WriteLockBlock writeLock(m_resPermCacheRWLock);
     MapResPermissionsCache::const_iterator i = m_resPermissionsMap.find( user ); 
     CResPermissionsCache* pResPermissionsCache;
 
@@ -282,7 +301,7 @@ void CPermissionsCache::removePermissions( ISecUser& sec_user)
 #ifdef _DEBUG
         DBGLOG("CACHE: CPermissionsCache Removing permissions for user %s", user);
 #endif
-        CriticalBlock block(m_resPermCacheLock);
+        WriteLockBlock writeLock(m_resPermCacheRWLock);
         m_resPermissionsMap.erase(user); 
     }
 }
@@ -290,7 +309,7 @@ void CPermissionsCache::removePermissions( ISecUser& sec_user)
 void CPermissionsCache::remove(SecResourceType rtype, const char* resourcename)
 {
     MapResPermissionsCache::const_iterator i;
-    CriticalBlock block(m_resPermCacheLock);
+    WriteLockBlock writeLock(m_resPermCacheRWLock);
     MapResPermissionsCache::const_iterator iEnd = m_resPermissionsMap.end(); 
 
     for (i = m_resPermissionsMap.begin(); i != iEnd; i++)
@@ -310,44 +329,57 @@ bool CPermissionsCache::lookup(ISecUser& sec_user)
         return false;
 
     string key(username);
-    CriticalBlock block(m_userCacheLock);
-
-    MapUserCache::iterator it = m_userCache.find(key);
-    if (it == m_userCache.end())
-        return false;
-    CachedUser* user = (CachedUser*)(it->second);
-
-    time_t now;
-    time(&now);
-    if(user->getTimestamp() < (now - m_cacheTimeout))
+    bool deleteEntry = false;
     {
-        m_userCache.erase(username);
-        delete user;
-        return false;
-    }
+        ReadLockBlock readLock(m_userCacheRWLock );
 
-    const char* cachedpw = user->queryUser()->credentials().getPassword();
-    StringBuffer pw(sec_user.credentials().getPassword());
-    
-    if(cachedpw && pw.length() > 0)
-    {
-        StringBuffer md5pbuf;
-        md5_string(pw, md5pbuf);
-        if(strcmp(cachedpw, md5pbuf.str()) == 0)
+        MapUserCache::iterator it = m_userCache.find(key);
+        if (it == m_userCache.end())
+            return false;
+        CachedUser* user = (CachedUser*)(it->second);
+
+        time_t now;
+        time(&now);
+        if(user->getTimestamp() < (now - m_cacheTimeout))
         {
+            deleteEntry = true;
+        }
+        else
+        {
+            const char* cachedpw = user->queryUser()->credentials().getPassword();
+            StringBuffer pw(sec_user.credentials().getPassword());
+
+            if(cachedpw && pw.length() > 0)
+            {
+                StringBuffer md5pbuf;
+                md5_string(pw, md5pbuf);
+                if(strcmp(cachedpw, md5pbuf.str()) == 0)
+                {
 #ifdef _DEBUG
-            DBGLOG("CACHE: CPermissionsCache Found validated user %s", username);
+                    DBGLOG("CACHE: CPermissionsCache Found validated user %s", username);
 #endif
-            // Copy cached user to the sec_user structure, but still keep the original clear text password.
-            user->queryUser()->copyTo(sec_user);
-            sec_user.credentials().setPassword(pw.str());
-            return true;
+                    // Copy cached user to the sec_user structure, but still keep the original clear text password.
+                    user->queryUser()->copyTo(sec_user);
+                    sec_user.credentials().setPassword(pw.str());
+                    return true;
+                }
+                else
+                {
+                    deleteEntry = true;
+                }
+            }
         }
-        else
+    }
+
+    if (deleteEntry)
+    {
+        WriteLockBlock writeLock(m_userCacheRWLock);
+        MapUserCache::iterator it = m_userCache.find(key);
+        if (it != m_userCache.end())
         {
+            CachedUser* user = (CachedUser*)(it->second);
             m_userCache.erase(username);
             delete user;
-            return false;
         }
     }
 
@@ -364,7 +396,7 @@ ISecUser* CPermissionsCache::getCachedUser( ISecUser& sec_user)
         return NULL;
 
     string key(username);
-    CriticalBlock block(m_userCacheLock);
+    ReadLockBlock readLock(m_userCacheRWLock );
     MapUserCache::iterator it = m_userCache.find(key);
     if (it == m_userCache.end())
         return NULL;
@@ -382,7 +414,7 @@ void CPermissionsCache::add(ISecUser& sec_user)
         return;
     
     string key(username);
-    CriticalBlock block(m_userCacheLock);
+    WriteLockBlock writeLock(m_userCacheRWLock );
     MapUserCache::iterator it = m_userCache.find(key);
     CachedUser* user = NULL;
     if (it != m_userCache.end())
@@ -403,7 +435,7 @@ void CPermissionsCache::removeFromUserCache(ISecUser& sec_user)
     if(username && *username)
     {
         string key(username);
-        CriticalBlock block(m_userCacheLock);
+        WriteLockBlock writeLock(m_userCacheRWLock );
         MapUserCache::iterator it = m_userCache.find(key);
         if (it != m_userCache.end())
         {
@@ -419,7 +451,7 @@ void CPermissionsCache::removeFromUserCache(ISecUser& sec_user)
 
 bool CPermissionsCache::addManagedFileScopes(IArrayOf<ISecResource>& scopes)
 {
-    CriticalBlock block(m_scopesLock);
+    WriteLockBlock writeLock(m_scopesRWLock);
     ForEachItemIn(x, scopes)
     {
         ISecResource* scope = &scopes.item(x);
@@ -445,7 +477,7 @@ bool CPermissionsCache::addManagedFileScopes(IArrayOf<ISecResource>& scopes)
 
 inline void CPermissionsCache::removeManagedFileScopes(IArrayOf<ISecResource>& scopes)
 {
-    CriticalBlock block(m_scopesLock);
+    WriteLockBlock writeLock(m_scopesRWLock);
     ForEachItemIn(x, scopes)
     {
         ISecResource* scope = &scopes.item(x);
@@ -466,7 +498,7 @@ inline void CPermissionsCache::removeManagedFileScopes(IArrayOf<ISecResource>& s
 
 inline void CPermissionsCache::removeAllManagedFileScopes()
 {
-    CriticalBlock block(m_scopesLock);
+    WriteLockBlock writeLock(m_scopesRWLock);
     map<string, ISecResource*>::const_iterator cit;
     map<string, ISecResource*>::const_iterator iEnd = m_managedFileScopesMap.end();
 
@@ -538,7 +570,7 @@ bool CPermissionsCache::queryPermsManagedFileScope(ISecUser& sec_user, const cha
     ISecResource *matchedRes = NULL;
     ISecResource *res = NULL;
     bool isManaged = false;
-    CriticalBlock block(m_scopesLock);
+    ReadLockBlock readLock(m_scopesRWLock);
     for(unsigned i = 0; i < scopes.length(); i++)
     {
         const char* scope = scopes.item(i);
@@ -619,7 +651,7 @@ void CPermissionsCache::flush()
     // MORE - is this safe? m_defaultPermossion and m_lastManagedFileScopesRefresh are unprotected,
     // and entries could be added to the first cache while the second is being cleared - does that matter?
     {
-        CriticalBlock block(m_resPermCacheLock);
+        WriteLockBlock writeLock(m_resPermCacheRWLock);
         MapResPermissionsCache::const_iterator i;
         MapResPermissionsCache::const_iterator iEnd = m_resPermissionsMap.end();
         for (i = m_resPermissionsMap.begin(); i != iEnd; i++)
@@ -627,7 +659,7 @@ void CPermissionsCache::flush()
         m_resPermissionsMap.clear();
     }
     {
-        CriticalBlock block(m_userCacheLock);
+        WriteLockBlock writeLock(m_userCacheRWLock );
         MapUserCache::const_iterator ui;
         MapUserCache::const_iterator uiEnd = m_userCache.end();
         for (ui = m_userCache.begin(); ui != uiEnd; ui++)

system/security/shared/caching.hpp

@@ -70,10 +70,11 @@ public:
     virtual void add( IArrayOf<ISecResource>& resources );
     virtual void remove(SecResourceType rtype, const char* resourcename);
 
-private:
     //removes entries older than tstamp passed in
     //
     virtual void removeStaleEntries(time_t tstamp);
+private:
+
 
     //type definitions
     //define mapping from resource name to pair<timeout, permission>
@@ -191,20 +192,20 @@ private:
     typedef std::map<string, CachedUser*> MapUserCache;
 
     MapResPermissionsCache m_resPermissionsMap;  //user specific resource permissions cache
-    mutable CriticalSection m_resPermCacheLock; //guards m_resPermissionsMap - DO NOT use RW lock as the lookup modifies the cache to remove expired entries
+    mutable ReadWriteLock m_resPermCacheRWLock; //guards m_resPermissionsMap
 
     int m_cacheTimeout; //cleanup cycle period
     bool m_transactionalEnabled;
 
     MapUserCache m_userCache;
-    mutable CriticalSection m_userCacheLock;    //guards m_userCache - DO NOT use RW lock as the lookup modifies the cache to remove expired entries
+    mutable ReadWriteLock m_userCacheRWLock;    //guards m_userCache
 
     StringAttr                  m_secMgrClass;
 
     //Managed File Scope support
     int                         m_defaultPermission;
     map<string, ISecResource*>  m_managedFileScopesMap;
-    mutable CriticalSection     m_scopesLock;  //guards m_managedFileScopesMap
+    mutable ReadWriteLock       m_scopesRWLock;//guards m_managedFileScopesMap
     ISecManager *               m_secMgr;
     time_t                      m_lastManagedFileScopesRefresh;
 };