https://issues.shibboleth.net/jira/browse/SSPCPP-507

diff --git a/odbc-store/odbc-store.cpp b/odbc-store/odbc-store.cpp
index 4357276..48bbe8e 100644 (file)
--- a/odbc-store/odbc-store.cpp
+++ b/odbc-store/odbc-store.cpp
@@ -63,7 +63,7 @@ using namespace boost;
 using namespace std;
 
 #define PLUGIN_VER_MAJOR 1
-#define PLUGIN_VER_MINOR 0
+#define PLUGIN_VER_MINOR 1
 
 #define LONGDATA_BUFLEN 16384
 
@@ -84,7 +84,7 @@ CREATE TABLE strings (
     context varchar(255) not null,
     id varchar(255) not null,
     expires datetime not null,
-    version smallint not null,
+    version int not null,
     value varchar(255) not null,
     PRIMARY KEY (context, id)
     )
@@ -93,7 +93,7 @@ CREATE TABLE texts (
     context varchar(255) not null,
     id varchar(255) not null,
     expires datetime not null,
-    version smallint not null,
+    version int not null,
     value text not null,
     PRIMARY KEY (context, id)
     )
@@ -207,6 +207,7 @@ namespace {
         SQLHENV m_henv;
         string m_connstring;
         long m_isolation;
+        bool m_wideVersion;
         vector<SQLINTEGER> m_retries;
     };
 
@@ -271,7 +272,7 @@ namespace {
 ODBCStorageService::ODBCStorageService(const DOMElement* e) : m_log(Category::getInstance("XMLTooling.StorageService")),
     m_caps(XMLHelper::getAttrInt(e, 255, contextSize), XMLHelper::getAttrInt(e, 255, keySize), XMLHelper::getAttrInt(e, 255, stringSize)),
     m_cleanupInterval(XMLHelper::getAttrInt(e, 900, cleanupInterval)),
-    cleanup_thread(nullptr), shutdown(false), m_henv(SQL_NULL_HENV), m_isolation(SQL_TXN_SERIALIZABLE)
+    cleanup_thread(nullptr), shutdown(false), m_henv(SQL_NULL_HENV), m_isolation(SQL_TXN_SERIALIZABLE), m_wideVersion(false)
 {
 #ifdef _DEBUG
     xmltooling::NDC ndc("ODBCStorageService");
@@ -321,6 +322,11 @@ ODBCStorageService::ODBCStorageService(const DOMElement* e) : m_log(Category::ge
         m_log.crit("unknown database version: %d.%d", v.first, v.second);
         throw XMLToolingException("Unknown database version for ODBC StorageService.");
     }
+    
+    if (v.first > 1 || v.second > 0) {
+        m_log.info("using 32-bit int type for version fields in tables");
+        m_wideVersion = true;
+    }
 
     // Load any retry errors to check.
     e = XMLHelper::getNextSiblingElement(e, RetryOnError);
@@ -542,20 +548,30 @@ int ODBCStorageService::readRow(const char *table, const char* context, const ch
     }
 
     SQLSMALLINT ver;
+    SQLINTEGER widever;
     SQL_TIMESTAMP_STRUCT expiration;
 
-    SQLBindCol(stmt, 1, SQL_C_SSHORT, &ver, 0, nullptr);
+    if (m_wideVersion)
+        SQLBindCol(stmt, 1, SQL_C_SLONG, &widever, 0, nullptr);
+    else
+        SQLBindCol(stmt, 1, SQL_C_SSHORT, &ver, 0, nullptr);
     if (pexpiration)
         SQLBindCol(stmt, 2, SQL_C_TYPE_TIMESTAMP, &expiration, 0, nullptr);
 
-    if ((sr = SQLFetch(stmt)) == SQL_NO_DATA)
+    if ((sr = SQLFetch(stmt)) == SQL_NO_DATA) {
+        if (m_log.isDebugEnabled())
+            m_log.debug("search returned no data (t=%s, c=%s, k=%s)", table, context, key);
         return 0;
+    }
 
     if (pexpiration)
         *pexpiration = timeFromTimestamp(expiration);
 
-    if (version == ver)
+    if (version == (m_wideVersion ? widever : ver)) {
+        if (m_log.isDebugEnabled())
+            m_log.debug("versioned search detected no change (t=%s, c=%s, k=%s)", table, context, key);
         return version; // nothing's changed, so just echo back the version
+    }
 
     if (pvalue) {
         SQLLEN len;
@@ -570,7 +586,7 @@ int ODBCStorageService::readRow(const char *table, const char* context, const ch
         }
     }
     
-    return ver;
+    return (m_wideVersion ? widever : ver);
 }
 
 int ODBCStorageService::updateRow(const char *table, const char* context, const char* key, const char* value, time_t expiration, int version)
@@ -608,15 +624,23 @@ int ODBCStorageService::updateRow(const char *table, const char* context, const
     }
 
     SQLSMALLINT ver;
-    SQLBindCol(stmt, 1, SQL_C_SSHORT, &ver, 0, nullptr);
+    SQLINTEGER widever;
+    if (m_wideVersion)
+        SQLBindCol(stmt, 1, SQL_C_SLONG, &widever, 0, nullptr);
+    else
+        SQLBindCol(stmt, 1, SQL_C_SSHORT, &ver, 0, nullptr);
     if ((sr = SQLFetch(stmt)) == SQL_NO_DATA) {
         return 0;
     }
 
     // Check version?
-    if (version > 0 && version != ver) {
+    if (version > 0 && version != (m_wideVersion ? widever : ver)) {
         return -1;
     }
+    else if ((m_wideVersion && widever == INT_MAX) || (!m_wideVersion && ver == 32767)) {
+        m_log.error("record version overflow (t=%s, c=%s, k=%s)", table, context, key);
+        throw IOException("Version overflow, record in ODBC StorageService could not be updated.");
+    }
 
     SQLFreeHandle(SQL_HANDLE_STMT, stmt);
     stmt = getHSTMT(conn);
@@ -651,11 +675,11 @@ int ODBCStorageService::updateRow(const char *table, const char* context, const
         else
             sr = SQLBindParam(stmt, 1, SQL_C_CHAR, SQL_VARCHAR, 255, 0, const_cast<char*>(value), &b_ind);
         if (!SQL_SUCCEEDED(sr)) {
-            m_log.error("SQLBindParam failed (context = %s)", context);
+            m_log.error("SQLBindParam failed (value = %s)", value);
             log_error(stmt, SQL_HANDLE_STMT);
             throw IOException("ODBC StorageService failed to update record.");
         }
-        m_log.debug("SQLBindParam succeeded (context = %s)", context);
+        m_log.debug("SQLBindParam succeeded (value = %s)", value);
     }
 
     int attempts = 3;
@@ -668,10 +692,10 @@ int ODBCStorageService::updateRow(const char *table, const char* context, const
             return 0;   // went missing?
         else if (SQL_SUCCEEDED(sr)) {
             m_log.debug("SQLExecute of update succeeded");
-            return ver + 1;
+            return (m_wideVersion ? widever : ver) + 1;
         }
 
-        m_log.error("update of record failed (t=%s, c=%s, k=%s", table, context, key);
+        m_log.error("update of record failed (t=%s, c=%s, k=%s)", table, context, key);
         logres = log_error(stmt, SQL_HANDLE_STMT);
     } while (attempts && logres.first);
 

diff --git a/shibsp/impl/StorageServiceSessionCache.cpp b/shibsp/impl/StorageServiceSessionCache.cpp
index f1af3f6..31c225a 100644 (file)
--- a/shibsp/impl/StorageServiceSessionCache.cpp
+++ b/shibsp/impl/StorageServiceSessionCache.cpp
@@ -133,7 +133,9 @@ namespace shibsp {
             const set<string>* indexes,
             time_t expires,
             vector<string>& sessions
-            );
+            ) {
+            return _logout(app, issuer, nameid, indexes, expires, sessions, 0);
+        }
         bool matches(
             const Application& app,
             const HTTPRequest& request,
@@ -203,7 +205,16 @@ namespace shibsp {
     private:
 #ifndef SHIBSP_LITE
         // maintain back-mappings of NameID/SessionIndex -> session key
-        void insert(const char* key, time_t expires, const char* name, const char* index);
+        void insert(const char* key, time_t expires, const char* name, const char* index, short attempts=0);
+        vector<string>::size_type _logout(
+            const Application& app,
+            const EntityDescriptor* issuer,
+            const saml2::NameID& nameid,
+            const set<string>* indexes,
+            time_t expires,
+            vector<string>& sessions,
+            short attempts
+            );
         bool stronglyMatches(const XMLCh* idp, const XMLCh* sp, const saml2::NameID& n1, const saml2::NameID& n2) const;
         LogoutEvent* newLogoutEvent(const Application& app) const;
 
@@ -560,6 +571,7 @@ void StoredSession::validate(const Application& app, const char* client_addr, ti
 
         // We may need to write back a new address into the session using a versioned update loop.
         if (client_addr) {
+            short attempts = 0;
             do {
                 const char* saddr = nullptr;
                 if (strchr(client_addr, ':'))
@@ -608,6 +620,10 @@ void StoredSession::validate(const Application& app, const char* client_addr, ti
                 }
                 else if (ver < 0) {
                     // Out of sync.
+                    if (++attempts > 10) {
+                        m_cache->m_log.error("failed to bind client address, update attempts exceeded limit");
+                        throw IOException("Unable to update stored session, exceeded retry limit.");
+                    }
                     m_cache->m_log.warn("storage service indicates the record is out of sync, updating with a fresh copy...");
                     ver = m_cache->m_storage->readText(getID(), "session", &record, nullptr);
                     if (!ver) {
@@ -654,6 +670,7 @@ void StoredSession::addAttributes(const vector<Attribute*>& attributes)
     m_cache->m_log.debug("adding attributes to session (%s)", getID());
 
     int ver;
+    short attempts = 0;
     do {
         DDF attr;
         DDF attrs = m_obj["attributes"];
@@ -696,6 +713,10 @@ void StoredSession::addAttributes(const vector<Attribute*>& attributes)
         }
         else if (ver < 0) {
             // Out of sync.
+            if (++attempts > 10) {
+                m_cache->m_log.error("failed to update stored session, update attempts exceeded limit");
+                throw IOException("Unable to update stored session, exceeded retry limit.");
+            }
             m_cache->m_log.warn("storage service indicates the record is out of sync, updating with a fresh copy...");
             ver = m_cache->m_storage->readText(getID(), "session", &record, nullptr);
             if (!ver) {
@@ -781,6 +802,7 @@ void StoredSession::addAssertion(Assertion* assertion)
         throw IOException("Attempted to insert duplicate assertion ID into session.");
 
     int ver;
+    short attempts = 0;
     do {
         DDF token = DDF(nullptr).string(id.get());
         m_obj["assertions"].add(token);
@@ -814,6 +836,10 @@ void StoredSession::addAssertion(Assertion* assertion)
         }
         else if (ver < 0) {
             // Out of sync.
+            if (++attempts > 10) {
+                m_cache->m_log.error("failed to update stored session, update attempts exceeded limit");
+                throw IOException("Unable to update stored session, exceeded retry limit.");
+            }
             m_cache->m_log.warn("storage service indicates the record is out of sync, updating with a fresh copy...");
             ver = m_cache->m_storage->readText(getID(), "session", &record, nullptr);
             if (!ver) {
@@ -1018,8 +1044,11 @@ void SSCache::test()
     m_storage->deleteString("SessionCacheTest", temp.get());
 }
 
-void SSCache::insert(const char* key, time_t expires, const char* name, const char* index)
+void SSCache::insert(const char* key, time_t expires, const char* name, const char* index, short attempts)
 {
+    if (attempts > 10)
+        throw IOException("Exceeded retry limit.");
+
     string dup;
     unsigned int storageLimit = m_storage_lite->getCapabilities().getKeySize();
     if (strlen(name) > storageLimit) {
@@ -1061,12 +1090,12 @@ void SSCache::insert(const char* key, time_t expires, const char* name, const ch
         ver = m_storage_lite->updateText("NameID", name, out.str().c_str(), max(expires, recordexp), ver);
         if (ver <= 0) {
             // Out of sync, or went missing, so retry.
-            return insert(key, expires, name, index);
+            return insert(key, expires, name, index, attempts + 1);
         }
     }
     else if (!m_storage_lite->createText("NameID", name, out.str().c_str(), expires)) {
         // Hit a dup, so just retry, hopefully hitting the other branch.
-        return insert(key, expires, name, index);
+        return insert(key, expires, name, index, attempts + 1);
     }
 }
 
@@ -1293,13 +1322,14 @@ bool SSCache::matches(
     return false;
 }
 
-vector<string>::size_type SSCache::logout(
+vector<string>::size_type SSCache::_logout(
     const Application& app,
     const saml2md::EntityDescriptor* issuer,
     const saml2::NameID& nameid,
     const set<string>* indexes,
     time_t expires,
-    vector<string>& sessionsKilled
+    vector<string>& sessionsKilled,
+    short attempts
     )
 {
 #ifdef _DEBUG
@@ -1308,6 +1338,8 @@ vector<string>::size_type SSCache::logout(
 
     if (!m_storage)
         throw ConfigurationException("SessionCache logout requires a StorageService.");
+    else if (attempts > 10)
+        throw IOException("Exceeded retry limit.");
 
     auto_ptr_char entityID(issuer ? issuer->getEntityID() : nullptr);
     auto_ptr_char name(nameid.getName());
@@ -1363,12 +1395,12 @@ vector<string>::size_type SSCache::logout(
             ver = m_storage_lite->updateText("Logout", name.get(), lout.str().c_str(), max(expires, oldexp), ver);
             if (ver <= 0) {
                 // Out of sync, or went missing, so retry.
-                return logout(app, issuer, nameid, indexes, expires, sessionsKilled);
+                return _logout(app, issuer, nameid, indexes, expires, sessionsKilled, attempts + 1);
             }
         }
         else if (!m_storage_lite->createText("Logout", name.get(), lout.str().c_str(), expires)) {
             // Hit a dup, so just retry, hopefully hitting the other branch.
-            return logout(app, issuer, nameid, indexes, expires, sessionsKilled);
+            return _logout(app, issuer, nameid, indexes, expires, sessionsKilled, attempts + 1);
         }
 
         obj.destroy();
@@ -1996,6 +2028,7 @@ void SSCache::receive(DDF& in, ostream& out)
 
         // We may need to write back a new address into the session using a versioned update loop.
         if (client_addr) {
+            short attempts = 0;
             m_log.info("binding session (%s) to new client address (%s)", key, client_addr);
             do {
                 // We have to reconstitute the session object ourselves.
@@ -2043,6 +2076,10 @@ void SSCache::receive(DDF& in, ostream& out)
                 }
                 if (ver < 0) {
                     // Out of sync.
+                    if (++attempts > 10) {
+                        m_log.error("failed to bind client address, update attempts exceeded limit");
+                        throw IOException("Unable to update stored session, exceeded retry limit.");
+                    }
                     m_log.warn("storage service indicates the record is out of sync, updating with a fresh copy...");
                     sessionobj["version"].integer(sessionobj["version"].integer() - 1);
                     ver = m_storage->readText(key, "session", &record);