[opensaml2] 21/38: CPPOST-95 - Dynamic metadata plugin race conditons / leaks
Ferenc Wágner
wferi at moszumanska.debian.org
Tue Aug 30 20:53:56 UTC 2016
This is an automated email from the git hooks/post-receive script.
wferi pushed a commit to branch master
in repository opensaml2.
commit 6e22d19e77ac07f8060b96787619d8f64c5a8d58
Author: Scott Cantor <cantor.2 at osu.edu>
Date: Wed Jun 1 15:46:27 2016 -0400
CPPOST-95 - Dynamic metadata plugin race conditons / leaks
https://issues.shibboleth.net/jira/browse/CPPOST-85
---
saml/saml2/metadata/AbstractMetadataProvider.h | 10 ++
saml/saml2/metadata/DynamicMetadataProvider.h | 10 ++
.../metadata/impl/AbstractMetadataProvider.cpp | 59 +++++-----
.../metadata/impl/DynamicMetadataProvider.cpp | 120 +++++++++++++++++++--
4 files changed, 167 insertions(+), 32 deletions(-)
diff --git a/saml/saml2/metadata/AbstractMetadataProvider.h b/saml/saml2/metadata/AbstractMetadataProvider.h
index d458b4c..211c082 100644
--- a/saml/saml2/metadata/AbstractMetadataProvider.h
+++ b/saml/saml2/metadata/AbstractMetadataProvider.h
@@ -102,6 +102,7 @@ namespace opensaml {
* Loads an entity into the cache for faster lookup.
* <p>This includes processing known reverse lookup strategies for artifacts.
* The validUntil parameter will contain the smallest value found on output.
+ * This method will *not* free any objects removed from the cache.</p>
*
* @param site entity definition
* @param validUntil maximum expiration time of the entity definition
@@ -122,6 +123,7 @@ namespace opensaml {
* @deprecated
* Loads an entity into the cache for faster lookup.
* <p>This includes processing known reverse lookup strategies for artifacts.
+ * This method will *not* free any objects removed from the cache.</p>
*
* @param site entity definition
* @param validUntil maximum expiration time of the entity definition
@@ -139,6 +141,14 @@ namespace opensaml {
virtual void index(EntitiesDescriptor* group, time_t validUntil) const;
/**
+ * Clear a specific entity from the cache.
+ *
+ * @param entityID the ID of the entity to remove
+ * @param freeSites true iff the objects cached in the site map should be freed.
+ */
+ virtual void unindex(const XMLCh* entityID, bool freeSites=false) const;
+
+ /**
* Clear the cache of known entities and groups.
*
* @param freeSites true iff the objects cached in the site map should be freed.
diff --git a/saml/saml2/metadata/DynamicMetadataProvider.h b/saml/saml2/metadata/DynamicMetadataProvider.h
index 2e19d7e..0f09bb5 100644
--- a/saml/saml2/metadata/DynamicMetadataProvider.h
+++ b/saml/saml2/metadata/DynamicMetadataProvider.h
@@ -30,7 +30,9 @@
#include <saml/saml2/metadata/AbstractMetadataProvider.h>
namespace xmltooling {
+ class XMLTOOL_API CondWait;
class XMLTOOL_API RWLock;
+ class XMLTOOL_API Thread;
};
namespace opensaml {
@@ -77,6 +79,14 @@ namespace opensaml {
time_t m_minCacheDuration, m_maxCacheDuration;
typedef std::map<xmltooling::xstring,time_t> cachemap_t;
mutable cachemap_t m_cacheMap;
+
+ // Used to manage background maintenance of cache.
+ bool m_shutdown;
+ time_t m_cleanupInterval;
+ time_t m_cleanupTimeout;
+ xmltooling::CondWait* m_cleanup_wait;
+ xmltooling::Thread* m_cleanup_thread;
+ static void* cleanup_fn(void*);
};
};
diff --git a/saml/saml2/metadata/impl/AbstractMetadataProvider.cpp b/saml/saml2/metadata/impl/AbstractMetadataProvider.cpp
index f2cca92..071933c 100644
--- a/saml/saml2/metadata/impl/AbstractMetadataProvider.cpp
+++ b/saml/saml2/metadata/impl/AbstractMetadataProvider.cpp
@@ -123,31 +123,10 @@ void AbstractMetadataProvider::indexEntity(EntityDescriptor* site, time_t& valid
auto_ptr_char id(site->getEntityID());
if (id.get()) {
if (replace) {
- // The data structure here needs work.
- // We have to find all the sites stored against the replaced ID. Then we have to
- // search for those sites in the entire set of sites tracked by the sources map and
- // remove them from both places.
- set<const EntityDescriptor*> existingSites;
- pair<sitemap_t::iterator,sitemap_t::iterator> existingRange = m_sites.equal_range(id.get());
- static pair<set<const EntityDescriptor*>::iterator,bool> (set<const EntityDescriptor*>::* ins)(const EntityDescriptor* const &) =
- &set<const EntityDescriptor*>::insert;
- for_each(
- existingRange.first, existingRange.second,
- lambda::bind(ins, boost::ref(existingSites), lambda::bind(&sitemap_t::value_type::second, _1))
- );
- m_sites.erase(existingRange.first, existingRange.second);
- for (sitemap_t::iterator s = m_sources.begin(); s != m_sources.end();) {
- if (existingSites.count(s->second) > 0) {
- sitemap_t::iterator temp = s;
- ++s;
- m_sources.erase(temp);
- }
- else {
- ++s;
- }
- }
+ // This won't free the old entries but will remove them from the cache.
+ unindex(site->getEntityID());
}
- m_sites.insert(sitemap_t::value_type(id.get(),site));
+ m_sites.insert(sitemap_t::value_type(id.get(), site));
}
// Process each IdP role.
@@ -240,6 +219,38 @@ void AbstractMetadataProvider::index(EntitiesDescriptor* group, time_t validUnti
indexGroup(group, validUntil);
}
+void AbstractMetadataProvider::unindex(const XMLCh* entityID, bool freeSites) const
+{
+ auto_ptr_char id(entityID);
+
+ // The data structure here needs work.
+ // We have to find all the sites stored against the replaced ID. Then we have to
+ // search for those sites in the entire set of sites tracked by the sources map and
+ // remove them from both places.
+ set<const EntityDescriptor*> existingSites;
+ pair<sitemap_t::iterator,sitemap_t::iterator> existingRange = m_sites.equal_range(id.get());
+ static pair<set<const EntityDescriptor*>::iterator,bool> (set<const EntityDescriptor*>::* ins)(const EntityDescriptor* const &) =
+ &set<const EntityDescriptor*>::insert;
+ for_each(
+ existingRange.first, existingRange.second,
+ lambda::bind(ins, boost::ref(existingSites), lambda::bind(&sitemap_t::value_type::second, _1))
+ );
+ m_sites.erase(existingRange.first, existingRange.second);
+ for (sitemap_t::iterator s = m_sources.begin(); s != m_sources.end();) {
+ if (existingSites.count(s->second) > 0) {
+ sitemap_t::iterator temp = s;
+ ++s;
+ m_sources.erase(temp);
+ }
+ else {
+ ++s;
+ }
+ }
+
+ if (freeSites)
+ for_each(existingSites.begin(), existingSites.end(), cleanup<EntityDescriptor>());
+}
+
void AbstractMetadataProvider::clearDescriptorIndex(bool freeSites)
{
if (freeSites)
diff --git a/saml/saml2/metadata/impl/DynamicMetadataProvider.cpp b/saml/saml2/metadata/impl/DynamicMetadataProvider.cpp
index 2768440..844788d 100644
--- a/saml/saml2/metadata/impl/DynamicMetadataProvider.cpp
+++ b/saml/saml2/metadata/impl/DynamicMetadataProvider.cpp
@@ -33,11 +33,18 @@
#include <xercesc/util/XMLUniDefs.hpp>
#include <xmltooling/logging.h>
#include <xmltooling/XMLToolingConfig.h>
+#include <xmltooling/util/NDC.h>
#include <xmltooling/util/ParserPool.h>
#include <xmltooling/util/Threads.h>
#include <xmltooling/util/XMLHelper.h>
#include <xmltooling/validation/ValidatorSuite.h>
+#if defined(XMLTOOLING_LOG4SHIB)
+# include <log4shib/NDC.hh>
+#elif defined(XMLTOOLING_LOG4CPP)
+# include <log4cpp/NDC.hh>
+#endif
+
using namespace opensaml::saml2md;
using namespace xmltooling::logging;
using namespace xmltooling;
@@ -48,6 +55,8 @@ using namespace std;
# endif
static const XMLCh id[] = UNICODE_LITERAL_2(i,d);
+static const XMLCh cleanupInterval[] = UNICODE_LITERAL_15(c,l,e,a,n,u,p,I,n,t,e,r,v,a,l);
+static const XMLCh cleanupTimeout[] = UNICODE_LITERAL_14(c,l,e,a,n,u,p,T,i,m,e,o,u,t);
static const XMLCh maxCacheDuration[] = UNICODE_LITERAL_16(m,a,x,C,a,c,h,e,D,u,r,a,t,i,o,n);
static const XMLCh minCacheDuration[] = UNICODE_LITERAL_16(m,i,n,C,a,c,h,e,D,u,r,a,t,i,o,n);
static const XMLCh refreshDelayFactor[] = UNICODE_LITERAL_18(r,e,f,r,e,s,h,D,e,l,a,y,F,a,c,t,o,r);
@@ -69,7 +78,11 @@ DynamicMetadataProvider::DynamicMetadataProvider(const DOMElement* e)
m_lock(RWLock::create()),
m_refreshDelayFactor(0.75),
m_minCacheDuration(XMLHelper::getAttrInt(e, 600, minCacheDuration)),
- m_maxCacheDuration(XMLHelper::getAttrInt(e, 28800, maxCacheDuration))
+ m_maxCacheDuration(XMLHelper::getAttrInt(e, 28800, maxCacheDuration)),
+ m_shutdown(false),
+ m_cleanupInterval(XMLHelper::getAttrInt(e, 1800, cleanupInterval)),
+ m_cleanupTimeout(XMLHelper::getAttrInt(e, 1800, cleanupTimeout)),
+ m_cleanup_wait(nullptr), m_cleanup_thread(nullptr)
{
if (m_minCacheDuration > m_maxCacheDuration) {
Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").error(
@@ -89,12 +102,93 @@ DynamicMetadataProvider::DynamicMetadataProvider(const DOMElement* e)
m_refreshDelayFactor = 0.75;
}
}
+
+ if (m_cleanupInterval > 0) {
+ if (m_cleanupTimeout < 0)
+ m_cleanupTimeout = 0;
+ m_cleanup_wait = CondWait::create();
+ m_cleanup_thread = Thread::create(&cleanup_fn, this);
+ }
}
DynamicMetadataProvider::~DynamicMetadataProvider()
{
// Each entity in the map is unique (no multimap semantics), so this is safe.
clearDescriptorIndex(true);
+
+ if (m_cleanup_thread) {
+ // Shut down the cleanup thread and let it know.
+ m_shutdown = true;
+ m_cleanup_wait->signal();
+ m_cleanup_thread->join(nullptr);
+ delete m_cleanup_thread;
+ delete m_cleanup_wait;
+ m_cleanup_thread = nullptr;
+ m_cleanup_wait = nullptr;
+ }
+}
+
+void* DynamicMetadataProvider::cleanup_fn(void* pv)
+{
+ DynamicMetadataProvider* provider = reinterpret_cast<DynamicMetadataProvider*>(pv);
+
+#ifndef WIN32
+ // First, let's block all signals
+ Thread::mask_all_signals();
+#endif
+
+ if (!provider->m_id.empty()) {
+ string threadid("[");
+ threadid += provider->m_id + ']';
+ logging::NDC::push(threadid);
+ }
+
+#ifdef _DEBUG
+ xmltooling::NDC ndc("cleanup");
+#endif
+
+ auto_ptr<Mutex> mutex(Mutex::create());
+ mutex->lock();
+
+ Category& log = Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic");
+
+ log.info("cleanup thread started...running every %d seconds", provider->m_cleanupInterval);
+
+ while (!provider->m_shutdown) {
+ provider->m_cleanup_wait->timedwait(mutex.get(), provider->m_cleanupInterval);
+ if (provider->m_shutdown)
+ break;
+
+ log.info("cleaning dynamic metadata cache...");
+
+ // Get a write lock.
+ provider->m_lock->wrlock();
+ SharedLock locker(provider->m_lock, false);
+
+ time_t now = time(nullptr);
+ // Dual iterator loop so we can remove entries while walking the map.
+ for (map<xstring, time_t>::iterator i = provider->m_cacheMap.begin(), i2 = i; i != provider->m_cacheMap.end(); i = i2) {
+ ++i2;
+ if (now > i->second + provider->m_cleanupTimeout) {
+ if (log.isDebugEnabled()) {
+ auto_ptr_char id(i->first.c_str());
+ log.debug("removing cache entry for (%s)", id.get());
+ }
+ provider->unindex(i->first.c_str(), true);
+ provider->m_cacheMap.erase(i);
+ }
+ }
+ }
+
+ log.info("cleanup thread finished");
+
+ mutex->unlock();
+
+ if (!provider->m_id.empty()) {
+ logging::NDC::pop();
+ }
+
+ return nullptr;
}
const XMLObject* DynamicMetadataProvider::getMetadata() const
@@ -126,6 +220,8 @@ pair<const EntityDescriptor*,const RoleDescriptor*> DynamicMetadataProvider::get
{
Category& log = Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic");
+ bool writeLocked = false;
+
// First we check the underlying cache.
pair<const EntityDescriptor*,const RoleDescriptor*> entity = AbstractMetadataProvider::getEntityDescriptor(criteria);
@@ -153,7 +249,6 @@ pair<const EntityDescriptor*,const RoleDescriptor*> DynamicMetadataProvider::get
if (cit != m_cacheMap.end()) {
if (time(nullptr) <= cit->second)
return entity;
- m_cacheMap.erase(cit);
}
string name;
@@ -230,6 +325,7 @@ pair<const EntityDescriptor*,const RoleDescriptor*> DynamicMetadataProvider::get
// Upgrade our lock so we can cache the new metadata.
m_lock->unlock();
m_lock->wrlock();
+ writeLocked = true;
// Notify observers.
emitChangeEvent(*entity2);
@@ -239,20 +335,22 @@ pair<const EntityDescriptor*,const RoleDescriptor*> DynamicMetadataProvider::get
// Make sure we clear out any existing copies, including stale metadata or if somebody snuck in.
cacheExp = SAMLTIME_MAX;
- indexEntity(entity2.get(), cacheExp, true);
+ unindex(entity2->getEntityID(), true); // actually frees the old instance with this ID
+ indexEntity(entity2.get(), cacheExp);
entity2.release();
m_lastUpdate = now;
-
- // Downgrade back to a read lock.
- m_lock->unlock();
- m_lock->rdlock();
}
catch (exception& e) {
log.error("error while resolving entityID (%s): %s", name.c_str(), e.what());
// This will return entries that are beyond their cache period,
// but not beyond their validity unless that criteria option was set.
- // If it is a cache-expired entry, bump the cache period to prevent retries.
+ // Bump the cache period to prevent retries, making sure we have a write lock
+ if (!writeLocked) {
+ m_lock->unlock();
+ m_lock->wrlock();
+ writeLocked = true;
+ }
if (entity.first)
m_cacheMap[entity.first->getEntityID()] = time(nullptr) + m_minCacheDuration;
else if (criteria.entityID_unicode)
@@ -265,6 +363,12 @@ pair<const EntityDescriptor*,const RoleDescriptor*> DynamicMetadataProvider::get
return entity;
}
+ // Downgrade back to a read lock.
+ if (writeLocked) {
+ m_lock->unlock();
+ m_lock->rdlock();
+ }
+
// Rinse and repeat.
return getEntityDescriptor(criteria);
}
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-shibboleth/opensaml2.git
More information about the Pkg-shibboleth-devel
mailing list