duke8253 commented on a change in pull request #7405:
URL: https://github.com/apache/trafficserver/pull/7405#discussion_r562070041
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -191,6 +191,12 @@ ssl_get_cached_session(SSL *ssl, const unsigned char *id,
int len, int *copy)
static int
ssl_new_cached_session(SSL *ssl, SSL_SESSION *sess)
{
+#if OPENSSL_VERSION_NUMBER >= 0x010101000L
Review comment:
yeah, I copied the version check from existing code, was trying to find
a better way to check versions later, but forgot to come back to it, will
update the code.
##########
File path: iocore/net/SSLSessionCache.h
##########
@@ -134,18 +166,19 @@ class SSLSessionBucket
public:
SSLSessionBucket();
~SSLSessionBucket();
- void insertSession(const SSLSessionID &, SSL_SESSION *ctx, SSL *ssl);
- bool getSession(const SSLSessionID &, SSL_SESSION **ctx,
ssl_session_cache_exdata **data);
- int getSessionBuffer(const SSLSessionID &, char *buffer, int &len);
- void removeSession(const SSLSessionID &);
+ void insertSession(const SSLSessionID &sid, SSL_SESSION *sess, SSL *ssl);
+ bool getSession(const SSLSessionID &sid, SSL_SESSION **sess,
ssl_session_cache_exdata **data);
+ int getSessionBuffer(const SSLSessionID &sid, char *buffer, int &len);
+ void removeSession(const SSLSessionID &sid);
private:
/* these method must be used while hold the lock */
void print(const char *) const;
- void removeOldestSession();
+ void removeOldestSession(const std::unique_lock<std::shared_mutex> &lock);
- Ptr<ProxyMutex> mutex;
- CountQueue<SSLSession> queue;
+ mutable std::shared_mutex mutex;
+ std::unordered_map<SSLSessionID, SSLSession *, SSLSessionIDHash> bucket_data;
+ std::map<ink_hrtime, SSLSession *> bucket_data_ts;
Review comment:
I was using a deque in the previous version, but then
`SSLSessionBucket::removeSession` will need to go through the entire queue to
delete an entry, which is why I switched to using a map.
##########
File path: iocore/net/SSLSessionCache.cc
##########
@@ -129,51 +129,50 @@ SSLSessionBucket::insertSession(const SSLSessionID &id,
SSL_SESSION *sess, SSL *
Debug("ssl.session_cache", "Inserting session '%s' to bucket %p.", buf,
this);
}
- MUTEX_TRY_LOCK(lock, mutex, this_ethread());
- if (!lock.is_locked()) {
+ Ptr<IOBufferData> buf;
+ Ptr<IOBufferData> buf_exdata;
+ size_t len_exdata = sizeof(ssl_session_cache_exdata);
+ buf = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
+ unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
+ i2d_SSL_SESSION(sess, &loc);
+ buf_exdata = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf_exdata->block_size()) >=
len_exdata);
+ ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata
*>(buf_exdata->data());
+ // This could be moved to a function in charge of populating exdata
+ exdata->curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl);
+ ink_hrtime now = Thread::get_hrtime_updated();
Review comment:
This is more of a way to make sure the keys in the map is always updated
with no duplication, such that the map and unordered_map always have the same
number of entries. Otherwise the `removeOldestSession` might not find a session
to remove, but in reality the session is still in the unordered_map, just not
in the map.
##########
File path: iocore/net/SSLSessionCache.cc
##########
@@ -129,51 +129,50 @@ SSLSessionBucket::insertSession(const SSLSessionID &id,
SSL_SESSION *sess, SSL *
Debug("ssl.session_cache", "Inserting session '%s' to bucket %p.", buf,
this);
}
- MUTEX_TRY_LOCK(lock, mutex, this_ethread());
- if (!lock.is_locked()) {
+ Ptr<IOBufferData> buf;
+ Ptr<IOBufferData> buf_exdata;
+ size_t len_exdata = sizeof(ssl_session_cache_exdata);
+ buf = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
+ unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
+ i2d_SSL_SESSION(sess, &loc);
+ buf_exdata = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf_exdata->block_size()) >=
len_exdata);
+ ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata
*>(buf_exdata->data());
+ // This could be moved to a function in charge of populating exdata
+ exdata->curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl);
+ ink_hrtime now = Thread::get_hrtime_updated();
+
+ ats_scoped_obj<SSLSession> ssl_session(new SSLSession(id, buf, len,
buf_exdata, now));
Review comment:
Actually I'm not sure why we create a new session object of our own
here, I checked the OpenSSL code and the reference count of the `SSL_SESSION`
is incremented when it goes into the new session callback. Which also means I
need to check if we're leaking memory here since I don't think we're calling
`SSL_SESSION_free`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
