Commit 3a8c98ea authored by Subodh Iyengar's avatar Subodh Iyengar Committed by Facebook Github Bot

add an option to clear error before calling ssl methods

Summary:
Normally clearing out the error before calling into an openssl method
is a bad idea. However there might be other code outside AsyncSSLSocket
calling into openssl in the same thread that doesn't use openssl
correctly. This allows users of AsyncSSLSocket to safeguard themselves
from such code.

Reviewed By: anirudhvr

Differential Revision: D4389970

fbshipit-source-id: 12da254d6b281c2b9d522ba19999b2489c0083a2
parent 266c8377
...@@ -22,8 +22,8 @@ ...@@ -22,8 +22,8 @@
#include <boost/noncopyable.hpp> #include <boost/noncopyable.hpp>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <openssl/err.h>
#include <openssl/asn1.h> #include <openssl/asn1.h>
#include <openssl/err.h>
#include <openssl/ssl.h> #include <openssl/ssl.h>
#include <sys/types.h> #include <sys/types.h>
#include <chrono> #include <chrono>
...@@ -1030,6 +1030,7 @@ AsyncSSLSocket::handleAccept() noexcept { ...@@ -1030,6 +1030,7 @@ AsyncSSLSocket::handleAccept() noexcept {
SSL_set_msg_callback_arg(ssl_, this); SSL_set_msg_callback_arg(ssl_, this);
} }
clearOpenSSLErrors();
int ret = SSL_accept(ssl_); int ret = SSL_accept(ssl_);
if (ret <= 0) { if (ret <= 0) {
int sslError; int sslError;
...@@ -1079,6 +1080,18 @@ AsyncSSLSocket::handleAccept() noexcept { ...@@ -1079,6 +1080,18 @@ AsyncSSLSocket::handleAccept() noexcept {
AsyncSocket::handleInitialReadWrite(); AsyncSocket::handleInitialReadWrite();
} }
void AsyncSSLSocket::clearOpenSSLErrors() {
// Normally clearing out the error before calling into an openssl method
// is a bad idea. However there might be other code that we don't control
// calling into openssl in the same thread, which doesn't use openssl
// correctly. We want to safe-guard ourselves from that code.
// However touching the ERR stack each and every time has a cost of taking
// a lock, so we only do this when we've opted in.
if (clearOpenSSLErrors_) {
ERR_clear_error();
}
}
void void
AsyncSSLSocket::handleConnect() noexcept { AsyncSSLSocket::handleConnect() noexcept {
VLOG(3) << "AsyncSSLSocket::handleConnect() this=" << this VLOG(3) << "AsyncSSLSocket::handleConnect() this=" << this
...@@ -1094,6 +1107,7 @@ AsyncSSLSocket::handleConnect() noexcept { ...@@ -1094,6 +1107,7 @@ AsyncSSLSocket::handleConnect() noexcept {
sslState_ == STATE_CONNECTING); sslState_ == STATE_CONNECTING);
assert(ssl_); assert(ssl_);
clearOpenSSLErrors();
auto originalState = state_; auto originalState = state_;
int ret = SSL_connect(ssl_); int ret = SSL_connect(ssl_);
if (ret <= 0) { if (ret <= 0) {
...@@ -1261,6 +1275,7 @@ AsyncSSLSocket::performRead(void** buf, size_t* buflen, size_t* offset) { ...@@ -1261,6 +1275,7 @@ AsyncSSLSocket::performRead(void** buf, size_t* buflen, size_t* offset) {
return AsyncSocket::performRead(buf, buflen, offset); return AsyncSocket::performRead(buf, buflen, offset);
} }
clearOpenSSLErrors();
int bytes = 0; int bytes = 0;
if (!isBufferMovable_) { if (!isBufferMovable_) {
bytes = SSL_read(ssl_, *buf, int(*buflen)); bytes = SSL_read(ssl_, *buf, int(*buflen));
......
...@@ -665,9 +665,20 @@ class AsyncSSLSocket : public virtual AsyncSocket { ...@@ -665,9 +665,20 @@ class AsyncSSLSocket : public virtual AsyncSocket {
return sessionResumptionAttempted_; return sessionResumptionAttempted_;
} }
/**
* Clears the ERR stack before invoking SSL methods.
* This is useful if unrelated code that runs in the same thread
* does not properly handle SSL error conditions, in which case
* it could cause SSL_* methods to fail with incorrect error codes.
*/
void setClearOpenSSLErrors(bool clearErr) {
clearOpenSSLErrors_ = clearErr;
}
private: private:
void init(); void init();
void clearOpenSSLErrors();
protected: protected:
...@@ -712,6 +723,7 @@ class AsyncSSLSocket : public virtual AsyncSocket { ...@@ -712,6 +723,7 @@ class AsyncSSLSocket : public virtual AsyncSocket {
// This virtual wrapper around SSL_write exists solely for testing/mockability // This virtual wrapper around SSL_write exists solely for testing/mockability
virtual int sslWriteImpl(SSL *ssl, const void *buf, int n) { virtual int sslWriteImpl(SSL *ssl, const void *buf, int n) {
clearOpenSSLErrors();
return SSL_write(ssl, buf, n); return SSL_write(ssl, buf, n);
} }
...@@ -822,6 +834,9 @@ class AsyncSSLSocket : public virtual AsyncSocket { ...@@ -822,6 +834,9 @@ class AsyncSSLSocket : public virtual AsyncSocket {
bool sessionResumptionAttempted_{false}; bool sessionResumptionAttempted_{false};
std::unique_ptr<IOBuf> preReceivedData_; std::unique_ptr<IOBuf> preReceivedData_;
// Whether or not to clear the err stack before invocation of another
// SSL method
bool clearOpenSSLErrors_{false};
}; };
} // namespace } // namespace
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