Commit 3289d2ed authored by Eric Hammerle's avatar Eric Hammerle Committed by Facebook Github Bot

Fix leak when calling AsyncSSLSocket::cancelConnect()

Summary: AsyncSSLSocket::connect() creates a new instance of AsyncSSLSocketConnector referred with a raw pointer and passes it to AsyncSocket::connect(). AsyncSSLSocket assumed a callback irrespective of connection success or failure so that this raw pointer will be properly disposed off.  However currently, if a connect attempt is cancelled the parent class may end up clearing the connect callback without invoking a connection error, thereby causing a leak. Since the cancelConnect() function also cancels the callback, the raw pointer should be cleaned up during cancellation.

Reviewed By: knekritz

Differential Revision: D18365027

fbshipit-source-id: 2cc171fc1a026b771dcb0bdd7c45b7ee450b97f7
parent 80d0f4b4
......@@ -83,6 +83,51 @@ inline bool zero_return(int error, int rc, int errno_copy) {
return false;
}
void setup_SSL_CTX(SSL_CTX* ctx) {
#ifdef SSL_MODE_RELEASE_BUFFERS
SSL_CTX_set_mode(
ctx,
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE |
SSL_MODE_RELEASE_BUFFERS);
#else
SSL_CTX_set_mode(
ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE);
#endif
// SSL_CTX_set_mode is a Macro
#ifdef SSL_MODE_WRITE_IOVEC
SSL_CTX_set_mode(ctx, SSL_CTX_get_mode(ctx) | SSL_MODE_WRITE_IOVEC);
#endif
}
// Note: This is a Leaky Meyer's Singleton. The reason we can't use a non-leaky
// thing is because we will be setting this BIO_METHOD* inside BIOs owned by
// various SSL objects which may get callbacks even during teardown. We may
// eventually try to fix this
BIO_METHOD* getSSLBioMethod() {
static auto const instance = OpenSSLUtils::newSocketBioMethod().release();
return instance;
}
void* initsslBioMethod() {
auto sslBioMethod = getSSLBioMethod();
// override the bwrite method for MSG_EOR support
OpenSSLUtils::setCustomBioWriteMethod(sslBioMethod, AsyncSSLSocket::bioWrite);
OpenSSLUtils::setCustomBioReadMethod(sslBioMethod, AsyncSSLSocket::bioRead);
// Note that the sslBioMethod.type and sslBioMethod.name are not
// set here. openssl code seems to be checking ".type == BIO_TYPE_SOCKET" and
// then have specific handlings. The sslWriteBioWrite should be compatible
// with the one in openssl.
// Return something here to enable AsyncSSLSocket to call this method using
// a function-scoped static.
return nullptr;
}
} // namespace
namespace folly {
class AsyncSSLSocketConnector : public AsyncSocket::ConnectCallback,
public AsyncSSLSocket::HandshakeCB {
private:
......@@ -91,9 +136,6 @@ class AsyncSSLSocketConnector : public AsyncSocket::ConnectCallback,
std::chrono::milliseconds timeout_;
std::chrono::steady_clock::time_point startTime_;
protected:
~AsyncSSLSocketConnector() override = default;
public:
AsyncSSLSocketConnector(
AsyncSSLSocket* sslSocket,
......@@ -104,6 +146,8 @@ class AsyncSSLSocketConnector : public AsyncSocket::ConnectCallback,
timeout_(timeout),
startTime_(std::chrono::steady_clock::now()) {}
~AsyncSSLSocketConnector() override = default;
void preConnect(folly::NetworkSocket fd) override {
VLOG(7) << "client preConnect hook is invoked";
if (callback_) {
......@@ -172,51 +216,6 @@ class AsyncSSLSocketConnector : public AsyncSocket::ConnectCallback,
}
};
void setup_SSL_CTX(SSL_CTX* ctx) {
#ifdef SSL_MODE_RELEASE_BUFFERS
SSL_CTX_set_mode(
ctx,
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE |
SSL_MODE_RELEASE_BUFFERS);
#else
SSL_CTX_set_mode(
ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE);
#endif
// SSL_CTX_set_mode is a Macro
#ifdef SSL_MODE_WRITE_IOVEC
SSL_CTX_set_mode(ctx, SSL_CTX_get_mode(ctx) | SSL_MODE_WRITE_IOVEC);
#endif
}
// Note: This is a Leaky Meyer's Singleton. The reason we can't use a non-leaky
// thing is because we will be setting this BIO_METHOD* inside BIOs owned by
// various SSL objects which may get callbacks even during teardown. We may
// eventually try to fix this
BIO_METHOD* getSSLBioMethod() {
static auto const instance = OpenSSLUtils::newSocketBioMethod().release();
return instance;
}
void* initsslBioMethod() {
auto sslBioMethod = getSSLBioMethod();
// override the bwrite method for MSG_EOR support
OpenSSLUtils::setCustomBioWriteMethod(sslBioMethod, AsyncSSLSocket::bioWrite);
OpenSSLUtils::setCustomBioReadMethod(sslBioMethod, AsyncSSLSocket::bioRead);
// Note that the sslBioMethod.type and sslBioMethod.name are not
// set here. openssl code seems to be checking ".type == BIO_TYPE_SOCKET" and
// then have specific handlings. The sslWriteBioWrite should be compatible
// with the one in openssl.
// Return something here to enable AsyncSSLSocket to call this method using
// a function-scoped static.
return nullptr;
}
} // namespace
namespace folly {
/**
* Create a client AsyncSSLSocket
*/
......@@ -704,12 +703,24 @@ void AsyncSSLSocket::connect(
noTransparentTls_ = true;
totalConnectTimeout_ = totalConnectTimeout;
if (sslState_ != STATE_UNENCRYPTED) {
callback = new AsyncSSLSocketConnector(this, callback, totalConnectTimeout);
allocatedConnectCallback_ =
new AsyncSSLSocketConnector(this, callback, totalConnectTimeout);
callback = allocatedConnectCallback_;
}
AsyncSocket::connect(
callback, address, int(connectTimeout.count()), options, bindAddr);
}
void AsyncSSLSocket::cancelConnect() {
if (connectCallback_ && allocatedConnectCallback_) {
// Since the connect callback won't be called, clean it up.
delete allocatedConnectCallback_;
allocatedConnectCallback_ = nullptr;
connectCallback_ = nullptr;
}
AsyncSocket::cancelConnect();
}
bool AsyncSSLSocket::needsPeerVerification() const {
if (verifyPeer_ == SSLContext::SSLVerifyPeerEnum::USE_CTX) {
return ctx_->needsPeerVerification();
......
......@@ -37,6 +37,8 @@
namespace folly {
class AsyncSSLSocketConnector;
/**
* A class for performing asynchronous I/O on an SSL connection.
*
......@@ -402,6 +404,15 @@ class AsyncSSLSocket : public virtual AsyncSocket {
using AsyncSocket::connect;
/**
* If a connect request is in-flight, cancels it and closes the socket
* immediately. Otherwise, this is a no-op.
*
* This does not invoke any connection related callbacks. Call this to
* prevent any connect callback while cleaning up, etc.
*/
void cancelConnect() override;
/**
* Initiate an SSL connection on the socket
* The callback will be invoked and uninstalled when an SSL connection
......@@ -785,6 +796,9 @@ class AsyncSSLSocket : public virtual AsyncSocket {
void init();
// Need to clean this up during a cancel if callback hasn't fired yet.
AsyncSSLSocketConnector* allocatedConnectCallback_;
protected:
/**
* Protected destructor.
......
......@@ -436,7 +436,7 @@ class AsyncSocket : virtual public AsyncTransportWrapper {
* This does not invoke any connection related callbacks. Call this to
* prevent any connect callback while cleaning up, etc.
*/
void cancelConnect();
virtual void cancelConnect();
/**
* Set the send timeout.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment