You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/26 20:12:20 UTC
[GitHub] [trafficserver] duke8253 opened a new pull request #7883: Origin session cache mem leak fix
duke8253 opened a new pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883
After running #7808 in our production for a while, we noticed there are memory leakage. Turns out ATS isn't following OpenSSL's documentation on how we store sessions externally, namely our `new_session_cb` (https://www.openssl.org/docs/manmaster/man3/SSL_CTX_sess_set_get_cb.html) always returns 0. So using the same method with the origin session cache caused the leak.
Mentioning #7479 to link all related PRs.
--
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:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 merged pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 merged pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#issuecomment-868716612
As mentioned above, we didn't follow OpenSSL's documentation on how we handle the new session callback. I tried making the callback for origin sessions do the same thing as server sessions, but looks like we deal with origin session differently too so the memory leak still happens even if I the call back returns 0 all the time, seems like we rely on openssl's own freeing mechanism on the origin side but not the server side. That's why I removed the whole `i2d_SSL_SESSION` conversion part. As for the `exdata`, as we only use it to store a single integer value, I see no reason to pass it as a pointer to a structure.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#discussion_r655963006
##########
File path: iocore/net/SSLSessionCache.h
##########
@@ -187,16 +187,16 @@ class SSLOriginSession
{
public:
std::string key;
- Ptr<IOBufferData> asn1_data; /* this is the ASN1 representation of the SSL_CTX */
- size_t len_asn1_data;
- Ptr<IOBufferData> extra_data;
+ SSL_SESSION *session;
+ ssl_curve_id curve_id;
- SSLOriginSession(const std::string &lookup_key, const Ptr<IOBufferData> &ssl_asn1_data, size_t len_asn1,
- Ptr<IOBufferData> &exdata)
- : key(lookup_key), asn1_data(ssl_asn1_data), len_asn1_data(len_asn1), extra_data(exdata)
+ SSLOriginSession(const std::string &lookup_key, SSL_SESSION *sess, ssl_curve_id curve)
+ : key(lookup_key), session(sess), curve_id(curve)
{
}
+ ~SSLOriginSession() { SSL_SESSION_free(session); }
Review comment:
Calling `SSL_SESSION_free` here is the fix, and other changes are not, right?
--
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:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#issuecomment-868712024
@duke8253 is going to take a look at the comment above.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#issuecomment-869925282
> My understanding of `i2d_SSL_SESSION` is just a serialization mechanism. So I think it should be possible to use i2d_SSL_SESSION without memory leak unless we are missing something. Is this correct?
> Removing the serialization part is fine if it's unnecessary, but is it only possible for origin sessions?
Correct, but when I tried to do the same for server sessions, weird things starts to happen: the sessions can only be reused once, and if I remember correctly new leaks started to happen after doing that, I'll have to check my notes on that. Which is why I'm saying we are doing special treatments in NetVC I'm not aware of. So for now removing the serialization can only be applied to origin sessions.
> Is this only true for origin sessions and we still need to pass it as a pointer to a structure for server sessions?
This is true for both origin sessions and server sessions, I didn't touch it in the server sessions as I'm not sure what is causing the problem I mentioned above. I'll make a PR once I figure out what's causing the problem.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#issuecomment-868796977
My understanding of `i2d_SSL_SESSION` is just a serialization mechanism. So I think it should be possible to use i2d_SSL_SESSION without memory leak unless we are missing something. Is this correct?
Removing the serialization part is fine if it's unnecessary, but is it only possible for origin sessions?
> As for the exdata, as we only use it to store a single integer value, I see no reason to pass it as a pointer to a structure.
Is this only true for origin sessions and we still need to pass it as a pointer to a structure for server sessions?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on a change in pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 commented on a change in pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#discussion_r658924101
##########
File path: iocore/net/SSLSessionCache.h
##########
@@ -187,16 +187,16 @@ class SSLOriginSession
{
public:
std::string key;
- Ptr<IOBufferData> asn1_data; /* this is the ASN1 representation of the SSL_CTX */
- size_t len_asn1_data;
- Ptr<IOBufferData> extra_data;
+ SSL_SESSION *session;
+ ssl_curve_id curve_id;
- SSLOriginSession(const std::string &lookup_key, const Ptr<IOBufferData> &ssl_asn1_data, size_t len_asn1,
- Ptr<IOBufferData> &exdata)
- : key(lookup_key), asn1_data(ssl_asn1_data), len_asn1_data(len_asn1), extra_data(exdata)
+ SSLOriginSession(const std::string &lookup_key, SSL_SESSION *sess, ssl_curve_id curve)
+ : key(lookup_key), session(sess), curve_id(curve)
{
}
+ ~SSLOriginSession() { SSL_SESSION_free(session); }
Review comment:
Not really, this is freeing the session when it's being replaced or removed from the cache, as we're not doing the `i2d_SSL_SESSION` conversion and returning the correct return value through the new session callback.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on a change in pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 commented on a change in pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#discussion_r658925732
##########
File path: iocore/net/SSLSessionCache.h
##########
@@ -187,16 +187,16 @@ class SSLOriginSession
{
public:
std::string key;
- Ptr<IOBufferData> asn1_data; /* this is the ASN1 representation of the SSL_CTX */
- size_t len_asn1_data;
- Ptr<IOBufferData> extra_data;
+ SSL_SESSION *session;
+ ssl_curve_id curve_id;
- SSLOriginSession(const std::string &lookup_key, const Ptr<IOBufferData> &ssl_asn1_data, size_t len_asn1,
- Ptr<IOBufferData> &exdata)
- : key(lookup_key), asn1_data(ssl_asn1_data), len_asn1_data(len_asn1), extra_data(exdata)
+ SSLOriginSession(const std::string &lookup_key, SSL_SESSION *sess, ssl_curve_id curve)
+ : key(lookup_key), session(sess), curve_id(curve)
{
}
+ ~SSLOriginSession() { SSL_SESSION_free(session); }
Review comment:
The reason for the leak is because when we do the `i2d_SSL_SESSION` for the server sessions, we always return 0 so the reference count for the actual OpenSSL's session object is decreased after returning from the callback, and we do some manual cleanups in our NetVC if we convert the session object back. But we seem to not do that for the origin sessions, and just rely on OpenSSL's own ref counter to clean up things.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #7883: Origin session cache mem leak fix
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #7883:
URL: https://github.com/apache/trafficserver/pull/7883#issuecomment-869925282
> My understanding of `i2d_SSL_SESSION` is just a serialization mechanism. So I think it should be possible to use i2d_SSL_SESSION without memory leak unless we are missing something. Is this correct?
> Removing the serialization part is fine if it's unnecessary, but is it only possible for origin sessions?
Correct, but when I tried to do the same for server sessions, weird things starts to happen: the sessions can only be reused once, and if I remember correctly new leaks started to happen after doing that, I'll have to check my notes on that. Which is why I'm saying we are doing special treatments in NetVC I'm not aware of. So for now removing the serialization can only be applied to origin sessions.
> Is this only true for origin sessions and we still need to pass it as a pointer to a structure for server sessions?
This is true for both origin sessions and server sessions, I didn't touch it in the server sessions as I'm not sure what is causing the problem I mentioned above. I'll make a PR once I figure out what's causing the problem.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org