shib-ccache.cpp:

  add another rwlock on the cachentry
  read-lock the cacheentry on find()
  aquire the write-lock() on the cacheentry before we try to remove() it

shib-target.h:
  add a release() method to a ccacheentry
  add a bunch of comments to the ccache methods

shibrpc-server.cpp:
  be sure to release() the ccacheentry when we're done with it

Fixes the race condition for deleting a cacheentry

diff --git a/shib-target/shib-ccache.cpp b/shib-target/shib-ccache.cpp
index f8758b5..677e3eb 100644 (file)
--- a/shib-target/shib-ccache.cpp
+++ b/shib-target/shib-ccache.cpp
@@ -105,9 +105,12 @@ public:
   virtual Iterator<SAMLAssertion*> getAssertions(Resource& resource);
   virtual bool isSessionValid(time_t lifetime, time_t timeout);
   virtual const char* getClientAddress() { return m_clientAddress.c_str(); }
+  virtual void release() { cacheitem_lock->unlock(); }
 
   void setCache(InternalCCache *cache) { m_cache = cache; }
   time_t lastAccess() { Lock lock(access_lock); return m_lastAccess; }
+  void rdlock() { cacheitem_lock->rdlock(); }
+  void wrlock() { cacheitem_lock->wrlock(); }
 
 private:
   ResourceEntry* populate(Resource& resource);
@@ -141,6 +144,7 @@ private:
 
   Mutex*       access_lock;
   RWLock*      resource_lock;
+  RWLock*      cacheitem_lock;
 
   class ResourceLock
   {
@@ -167,13 +171,16 @@ public:
                      const char *client_addr);
   virtual void remove(const char* key);
 
+  InternalCCacheEntry* findi(const char* key);
   void cleanup();
+
 private:
+  RWLock *lock;
+
   SAMLBinding* m_SAMLBinding;
   map<string,InternalCCacheEntry*> m_hashtable;
 
   log4cpp::Category* log;
-  RWLock *lock;
 
   static void* cleanup_fcn(void*); // XXX Assumed an InternalCCache
   bool         shutdown;
@@ -235,10 +242,10 @@ SAMLBinding* InternalCCache::getBinding(const XMLCh* bindingProt)
   return NULL;
 }
 
-CCacheEntry* InternalCCache::find(const char* key)
+// assumed a lock is held..
+InternalCCacheEntry* InternalCCache::findi(const char* key)
 {
-  log->debug("Find: \"%s\"", key);
-  ReadLock rwlock(lock);
+  log->debug("FindI: \"%s\"", key);
 
   map<string,InternalCCacheEntry*>::const_iterator i=m_hashtable.find(key);
   if (i==m_hashtable.end()) {
@@ -246,7 +253,21 @@ CCacheEntry* InternalCCache::find(const char* key)
     return NULL;
   }
   log->debug("Match Found.");
-  return dynamic_cast<CCacheEntry*>(i->second);
+
+  return i->second;
+}
+
+CCacheEntry* InternalCCache::find(const char* key)
+{
+  log->debug("Find: \"%s\"", key);
+  ReadLock rwlock(lock);
+
+  InternalCCacheEntry* entry = findi(key);
+  if (!entry) return NULL;
+
+  // Lock the database for the caller -- they have to release the item.
+  entry->rdlock();
+  return dynamic_cast<CCacheEntry*>(entry);
 }
 
 void InternalCCache::insert(const char* key, SAMLAuthenticationStatement *s,
@@ -262,15 +283,41 @@ void InternalCCache::insert(const char* key, SAMLAuthenticationStatement *s,
   lock->unlock();
 }
 
+// remove the entry from the database and then destroy the cacheentry
 void InternalCCache::remove(const char* key)
 {
   log->debug("removing cache entry \"key\"", key);
 
-  // XXX: FIXME? do we need to delete the CacheEntry?
+  // grab the entry from the database.  We'll have a readlock on it.
+  CCacheEntry* entry = findi(key);
+
+  if (!entry)
+    return;
 
+  // grab the cache write lock
   lock->wrlock();
+
+  // verify we've still got the same entry.
+  if (entry != findi(key)) {
+    // Nope -- must've already been removed.
+    lock->unlock();
+    return;
+  }
+
+  // ok, remove the entry.
   m_hashtable.erase(key);
   lock->unlock();
+
+  // now grab the write lock on the cacheitem.
+  // This will make sure all other threads have released this item.
+  InternalCCacheEntry* ientry = dynamic_cast<InternalCCacheEntry*>(entry);
+  ientry->wrlock();
+
+  // we can release immediately because we know we're not in the database!
+  ientry->release();
+
+  // Now delete the entry
+  delete ientry;
 }
 
 void InternalCCache::cleanup()
@@ -357,6 +404,7 @@ InternalCCacheEntry::InternalCCacheEntry(SAMLAuthenticationStatement *s, const c
   pop_locks_lock = Mutex::create();
   access_lock = Mutex::create();
   resource_lock = RWLock::create();
+  cacheitem_lock = RWLock::create();
 
   if (s == NULL) {
     log->error("NULL auth statement");
@@ -402,6 +450,7 @@ InternalCCacheEntry::~InternalCCacheEntry()
     delete i->second;
 
   delete pop_locks_lock;
+  delete cacheitem_lock;
   delete resource_lock;
   delete access_lock;
 }
@@ -534,6 +583,7 @@ void InternalCCacheEntry::insert(const char* resource, ResourceEntry* entry)
   resource_lock->unlock();
 }
 
+// caller will delete the entry.. don't worry about that here.
 void InternalCCacheEntry::remove(const char* resource)
 {
   log->debug("removing %s", resource);

diff --git a/shib-target/shib-target.h b/shib-target/shib-target.h
index 6fea206..53750a2 100644 (file)
--- a/shib-target/shib-target.h
+++ b/shib-target/shib-target.h
@@ -131,6 +131,7 @@ namespace shibtarget {
     virtual saml::Iterator<saml::SAMLAssertion*> getAssertions(Resource& resource) = 0;
     virtual bool isSessionValid(time_t lifetime, time_t timeout) = 0;
     virtual const char* getClientAddress() = 0;
+    virtual void release() = 0;
   };
     
   class CCache
@@ -139,11 +140,35 @@ namespace shibtarget {
     virtual ~CCache() = 0;
 
     virtual saml::SAMLBinding* getBinding(const XMLCh* bindingProt) = 0;
-    virtual CCacheEntry* find(const char* key) = 0;
+
+    // insert() the Auth Statement into the CCache.
+    //
+    // Make sure you do not hold any open CCacheEntry objects before
+    // you call this method.
+    //
     virtual void insert(const char* key, saml::SAMLAuthenticationStatement *s,
                        const char *client_addr) = 0;
+
+    // find() a CCacheEntry in the CCache for the given key.
+    //
+    // This returns a LOCKED cache entry.  You should release() it
+    // when you are done using it.
+    //
+    // Note that you MUST NOT call any other CCache methods while you
+    // are holding this CCacheEntry!
+    //
+    virtual CCacheEntry* find(const char* key) = 0;
+
+    // remove() a key from the CCache
+    //
+    // NOTE: If you previously executed a find(), make sure you
+    // "release()" the CCacheEntry before you try to remove it!
+    //
     virtual void remove(const char* key) = 0;
     
+    // create a CCache instance of the provided type.  A NULL type
+    // implies that it should create the default cache type.
+    //
     static CCache* getInstance(const char* type);
   };    
 

diff --git a/shib-target/shibrpc-server.cpp b/shib-target/shibrpc-server.cpp
index 10edffe..0c379cd 100644 (file)
--- a/shib-target/shibrpc-server.cpp
+++ b/shib-target/shibrpc-server.cpp
@@ -83,6 +83,7 @@ shibrpc_session_is_valid_1_svc(shibrpc_session_is_valid_args_1 *argp,
       result->status = SHIBRPC_IPADDR_MISMATCH;
       result->error_msg = 
        strdup ("Your IP address does not match the address in the original authentication.");
+      entry->release();
       g_shibTargetCCache->remove (argp->cookie.cookie);
       return TRUE;
     }
@@ -93,10 +94,13 @@ shibrpc_session_is_valid_1_svc(shibrpc_session_is_valid_args_1 *argp,
     log.debug ("Session expired");
     result->status = SHIBRPC_SESSION_EXPIRED;
     result->error_msg = strdup ("Your session has expired.  Re-authenticate.");
+    entry->release();
     g_shibTargetCCache->remove (argp->cookie.cookie);
     return TRUE;
   }
 
+  entry->release();
+
   // ok, we've succeeded..
   result->status = SHIBRPC_OK;
   result->error_msg = strdup("");
@@ -288,6 +292,7 @@ shibrpc_get_assertions_1_svc(shibrpc_get_assertions_args_1 *argp,
     result->status = SHIBRPC_IPADDR_MISMATCH;
     result->error_msg =
       strdup("Your IP address does not match the address in the original authentication.");
+    entry->release();
     return TRUE;
   }
 
@@ -321,9 +326,12 @@ shibrpc_get_assertions_1_svc(shibrpc_get_assertions_args_1 *argp,
     os << e;
     result->status = SHIBRPC_SAML_EXCEPTION;
     result->error_msg = strdup(os.str().c_str());
+    entry->release();
     return TRUE;
   }
 
+  entry->release();
+
   // and let it fly
   result->status = SHIBRPC_OK;
   result->error_msg = strdup("");