From 42ebd817a97a07af6887f456d1e972b3e53f3aba Mon Sep 17 00:00:00 2001 From: Fei Deng Date: Fri, 29 Oct 2021 00:20:41 -0500 Subject: [PATCH] use shared pointer to help with high memory utilization --- iocore/net/P_SSLNetVConnection.h | 2 +- iocore/net/SSLNetVConnection.cc | 28 +++++------- iocore/net/SSLSessionCache.cc | 56 ++++++++++++++--------- iocore/net/SSLSessionCache.h | 9 ++-- iocore/net/TLSSessionResumptionSupport.cc | 15 +++--- iocore/net/TLSSessionResumptionSupport.h | 2 +- 6 files changed, 57 insertions(+), 55 deletions(-) diff --git a/iocore/net/P_SSLNetVConnection.h b/iocore/net/P_SSLNetVConnection.h index 1691c0e5e7e..8e109a94d58 100644 --- a/iocore/net/P_SSLNetVConnection.h +++ b/iocore/net/P_SSLNetVConnection.h @@ -333,7 +333,7 @@ class SSLNetVConnection : public UnixNetVConnection, ink_hrtime sslLastWriteTime = 0; int64_t sslTotalBytesSent = 0; - SSL_SESSION *client_sess = nullptr; + std::shared_ptr client_sess = nullptr; // The serverName is either a pointer to the (null-terminated) name fetched from the // SSL object or the empty string. diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 74e4e10f37b..d556f4bc2b3 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -953,10 +953,9 @@ SSLNetVConnection::clear() // operation, e.g. by using d2i_SSL_SESSION(3). It must not be called // on other SSL_SESSION objects, as this would cause incorrect // reference counts and therefore program failures. - if (client_sess != nullptr) { - SSL_SESSION_free(client_sess); - client_sess = nullptr; - } + // Since we created the shared pointer with a custom deleter, + // resetting here will decrement the ref-counter. + client_sess.reset(); if (ssl != nullptr) { SSL_free(ssl); @@ -2089,16 +2088,11 @@ SSLNetVConnection::_ssl_connect() Debug("ssl.origin_session_cache", "origin session cache lookup key = %s", lookup_key.c_str()); - sess = this->getOriginSession(ssl, lookup_key); - if (sess) { - if (SSL_set_session(ssl, sess) == 0) { - SSL_SESSION_free(sess); - } else { - if (this->client_sess) { - SSL_SESSION_free(this->client_sess); - } - this->client_sess = sess; - } + std::shared_ptr shared_sess = this->getOriginSession(ssl, lookup_key); + + if (shared_sess && SSL_set_session(ssl, shared_sess.get())) { + // Keep a reference of this shared pointer in the connection + this->client_sess = shared_sess; } } } @@ -2106,14 +2100,14 @@ SSLNetVConnection::_ssl_connect() int ret = SSL_connect(ssl); if (ret > 0) { - if (sess && SSL_session_reused(ssl)) { + if (SSL_session_reused(ssl)) { SSL_INCREMENT_DYN_STAT(ssl_origin_session_reused_count); if (is_debug_tag_set("ssl.origin_session_cache")) { - Debug("ssl.origin_session_cache", "reused session to origin server = %p", sess); + Debug("ssl.origin_session_cache", "reused session to origin server"); } } else { if (is_debug_tag_set("ssl.origin_session_cache")) { - Debug("ssl.origin_session_cache", "new session to origin server = %p", sess); + Debug("ssl.origin_session_cache", "new session to origin server"); } } return SSL_ERROR_NONE; diff --git a/iocore/net/SSLSessionCache.cc b/iocore/net/SSLSessionCache.cc index 1bcc52f41c7..eda95025a5d 100644 --- a/iocore/net/SSLSessionCache.cc +++ b/iocore/net/SSLSessionCache.cc @@ -310,6 +310,13 @@ SSLSessionBucket::removeSession(const SSLSessionID &id) return; } +// Custom deleter for shared origin sessions +void +SSLSessDeleter(SSL_SESSION *_p) +{ + SSL_SESSION_free(_p); +} + /* Session Bucket */ SSLSessionBucket::SSLSessionBucket() {} @@ -322,10 +329,6 @@ SSLOriginSessionCache::~SSLOriginSessionCache() {} void SSLOriginSessionCache::insert_session(const std::string &lookup_key, SSL_SESSION *sess, SSL *ssl) { - if (is_debug_tag_set("ssl.origin_session_cache")) { - Debug("ssl.origin_session_cache", "insert session: %s = %p", lookup_key.c_str(), sess); - } - size_t len = i2d_SSL_SESSION(sess, nullptr); // make sure we're not going to need more than SSL_MAX_ORIG_SESSION_SIZE bytes /* do not cache a session that's too big. */ @@ -338,23 +341,34 @@ SSLOriginSessionCache::insert_session(const std::string &lookup_key, SSL_SESSION return; } - Ptr buf; - buf = new_IOBufferData(buffer_size_to_index(len, MAX_BUFFER_SIZE_INDEX), MEMALIGNED); - ink_release_assert(static_cast(buf->block_size()) >= len); - unsigned char *loc = reinterpret_cast(buf->data()); - i2d_SSL_SESSION(sess, &loc); + // Duplicate the session from the connection, we'll be keeping track the ref-count with a shared pointer ourself + SSL_SESSION *sess_ptr = SSL_SESSION_dup(sess); + + if (is_debug_tag_set("ssl.origin_session_cache")) { + Debug("ssl.origin_session_cache", "insert session: %s = %p", lookup_key.c_str(), sess_ptr); + } + + // Create the shared pointer to the session, with the custom deleter + std::shared_ptr shared_sess(sess_ptr, SSLSessDeleter); ssl_curve_id curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl); - ats_scoped_obj ssl_orig_session(new SSLOriginSession(lookup_key, buf, len, curve)); + ats_scoped_obj ssl_orig_session(new SSLOriginSession(lookup_key, curve, shared_sess)); auto new_node = ssl_orig_session.release(); std::unique_lock lock(mutex); auto entry = orig_sess_map.find(lookup_key); if (entry != orig_sess_map.end()) { auto node = entry->second; + if (is_debug_tag_set("ssl.origin_session_cache")) { + Debug("ssl.origin_session_cache", "found duplicate key: %s, replacing %p with %p", lookup_key.c_str(), + node->shared_sess.get(), sess_ptr); + } orig_sess_que.remove(node); orig_sess_map.erase(entry); delete node; } else if (orig_sess_map.size() >= SSLConfigParams::origin_session_cache_size) { + if (is_debug_tag_set("ssl.origin_session_cache")) { + Debug("ssl.origin_session_cache", "origin session cache full, removing oldest session"); + } remove_oldest_session(lock); } @@ -362,8 +376,8 @@ SSLOriginSessionCache::insert_session(const std::string &lookup_key, SSL_SESSION orig_sess_map[lookup_key] = new_node; } -bool -SSLOriginSessionCache::get_session(const std::string &lookup_key, SSL_SESSION **sess, ssl_curve_id *curve) +std::shared_ptr +SSLOriginSessionCache::get_session(const std::string &lookup_key, ssl_curve_id *curve) { if (is_debug_tag_set("ssl.origin_session_cache")) { Debug("ssl.origin_session_cache", "get session: %s", lookup_key.c_str()); @@ -372,27 +386,26 @@ SSLOriginSessionCache::get_session(const std::string &lookup_key, SSL_SESSION ** std::shared_lock lock(mutex); auto entry = orig_sess_map.find(lookup_key); if (entry == orig_sess_map.end()) { - return false; + return nullptr; } - const unsigned char *loc = reinterpret_cast(entry->second->asn1_data->data()); - *sess = d2i_SSL_SESSION(nullptr, &loc, entry->second->len_asn1_data); if (curve != nullptr) { *curve = entry->second->curve_id; } - return true; + + return entry->second->shared_sess; } void SSLOriginSessionCache::remove_oldest_session(const std::unique_lock &lock) { // Caller must hold the bucket shared_mutex with unique_lock. - ink_assert(lock.owns_lock()); + ink_release_assert(lock.owns_lock()); while (orig_sess_que.head && orig_sess_que.size >= static_cast(SSLConfigParams::origin_session_cache_size)) { auto node = orig_sess_que.pop(); if (is_debug_tag_set("ssl.origin_session_cache")) { - Debug("ssl.origin_session_cache", "remove oldest session: %s", node->key.c_str()); + Debug("ssl.origin_session_cache", "remove oldest session: %s, session ptr: %p", node->key.c_str(), node->shared_sess.get()); } orig_sess_map.erase(node->key); delete node; @@ -403,14 +416,13 @@ void SSLOriginSessionCache::remove_session(const std::string &lookup_key) { // We can't bail on contention here because this session MUST be removed. - if (is_debug_tag_set("ssl.origin_session_cache")) { - Debug("ssl.origin_session_cache", "remove session: %s", lookup_key.c_str()); - } - std::unique_lock lock(mutex); auto entry = orig_sess_map.find(lookup_key); if (entry != orig_sess_map.end()) { auto node = entry->second; + if (is_debug_tag_set("ssl.origin_session_cache")) { + Debug("ssl.origin_session_cache", "remove session: %s, session ptr: %p", lookup_key.c_str(), node->shared_sess.get()); + } orig_sess_que.remove(node); orig_sess_map.erase(entry); delete node; diff --git a/iocore/net/SSLSessionCache.h b/iocore/net/SSLSessionCache.h index 2a099d36c1e..01edf7662eb 100644 --- a/iocore/net/SSLSessionCache.h +++ b/iocore/net/SSLSessionCache.h @@ -188,12 +188,11 @@ class SSLOriginSession { public: std::string key; - Ptr asn1_data; /* this is the ASN1 representation of the SSL_CTX */ - size_t len_asn1_data; ssl_curve_id curve_id; + std::shared_ptr shared_sess = nullptr; - SSLOriginSession(const std::string &lookup_key, const Ptr &asn1, size_t len_asn1, ssl_curve_id curve) - : key(lookup_key), asn1_data(asn1), len_asn1_data(len_asn1), curve_id(curve) + SSLOriginSession(const std::string &lookup_key, ssl_curve_id curve, std::shared_ptr session) + : key(lookup_key), curve_id(curve), shared_sess(session) { } @@ -207,7 +206,7 @@ class SSLOriginSessionCache ~SSLOriginSessionCache(); void insert_session(const std::string &lookup_key, SSL_SESSION *sess, SSL *ssl); - bool get_session(const std::string &lookup_key, SSL_SESSION **sess, ssl_curve_id *curve); + std::shared_ptr get_session(const std::string &lookup_key, ssl_curve_id *curve); void remove_session(const std::string &lookup_key); private: diff --git a/iocore/net/TLSSessionResumptionSupport.cc b/iocore/net/TLSSessionResumptionSupport.cc index 605d9a8be75..af22b2e8959 100644 --- a/iocore/net/TLSSessionResumptionSupport.cc +++ b/iocore/net/TLSSessionResumptionSupport.cc @@ -173,20 +173,17 @@ TLSSessionResumptionSupport::getSession(SSL *ssl, const unsigned char *id, int l return session; } -SSL_SESSION * +std::shared_ptr TLSSessionResumptionSupport::getOriginSession(SSL *ssl, const std::string &lookup_key) { - SSL_SESSION *session = nullptr; - ssl_curve_id curve = 0; - if (origin_sess_cache->get_session(lookup_key, &session, &curve)) { - ink_assert(session); + ssl_curve_id curve = 0; + std::shared_ptr shared_sess = origin_sess_cache->get_session(lookup_key, &curve); + if (shared_sess != nullptr) { // Double check the timeout - if (is_ssl_session_timed_out(session)) { + if (is_ssl_session_timed_out(shared_sess.get())) { SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_miss); origin_sess_cache->remove_session(lookup_key); - SSL_SESSION_free(session); - session = nullptr; } else { SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_hit); this->_setSSLSessionCacheHit(true); @@ -195,7 +192,7 @@ TLSSessionResumptionSupport::getOriginSession(SSL *ssl, const std::string &looku } else { SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_miss); } - return session; + return shared_sess; } void diff --git a/iocore/net/TLSSessionResumptionSupport.h b/iocore/net/TLSSessionResumptionSupport.h index e91847a25db..5f1fa37a91c 100644 --- a/iocore/net/TLSSessionResumptionSupport.h +++ b/iocore/net/TLSSessionResumptionSupport.h @@ -51,7 +51,7 @@ class TLSSessionResumptionSupport ssl_curve_id getSSLCurveNID() const; SSL_SESSION *getSession(SSL *ssl, const unsigned char *id, int len, int *copy); - SSL_SESSION *getOriginSession(SSL *ssl, const std::string &lookup_key); + std::shared_ptr getOriginSession(SSL *ssl, const std::string &lookup_key); protected: void clear();