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

diff --git a/folly/io/async/AsyncSSLSocket.cpp b/folly/io/async/AsyncSSLSocket.cpp
index 7910bd02aca6adc11e6d38a8949304ec3c726908..cb4b96d2ba9c78b589eef77aceb83e51393f902e 100644 (file)
--- a/folly/io/async/AsyncSSLSocket.cpp
+++ b/folly/io/async/AsyncSSLSocket.cpp
@@ -22,8 +22,8 @@
 #include <boost/noncopyable.hpp>
 #include <errno.h>
 #include <fcntl.h>
 #include <boost/noncopyable.hpp>
 #include <errno.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 <sys/types.h>
 #include <chrono>
 #include <openssl/ssl.h>
 #include <sys/types.h>
 #include <chrono>


@@ -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_);
   if (ret <= 0) {
     int sslError;
   int ret = SSL_accept(ssl_);
   if (ret <= 0) {
     int sslError;


@@ -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
 AsyncSSLSocket::handleConnect() noexcept {
   VLOG(3) <<  "AsyncSSLSocket::handleConnect() this=" << this
 void
 AsyncSSLSocket::handleConnect() noexcept {
   VLOG(3) <<  "AsyncSSLSocket::handleConnect() this=" << this


@@ -1094,6 +1107,7 @@ AsyncSSLSocket::handleConnect() noexcept {
       sslState_ == STATE_CONNECTING);
   assert(ssl_);
 
       sslState_ == STATE_CONNECTING);
   assert(ssl_);
 

+  clearOpenSSLErrors();

   auto originalState = state_;
   int ret = SSL_connect(ssl_);
   if (ret <= 0) {
   auto originalState = state_;
   int ret = SSL_connect(ssl_);
   if (ret <= 0) {


@@ -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;
   if (!isBufferMovable_) {
     bytes = SSL_read(ssl_, *buf, int(*buflen));
   int bytes = 0;
   if (!isBufferMovable_) {
     bytes = SSL_read(ssl_, *buf, int(*buflen));

diff --git a/folly/io/async/AsyncSSLSocket.h b/folly/io/async/AsyncSSLSocket.h
index 01ff7bd2098ea0abe67994fa90d38b362815c40e..e7d499e9f2ceeb01f12bffe84d99d5c7f26e3ab7 100644 (file)
--- a/folly/io/async/AsyncSSLSocket.h
+++ b/folly/io/async/AsyncSSLSocket.h
@@ -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:
 
   void init();
  private:
 
   void init();

+  void clearOpenSSLErrors();

 
  protected:
 
 
  protected:
 


@@ -712,6 +723,7 @@ class AsyncSSLSocket : public virtual AsyncSocket {
 
   // This virtual wrapper around SSL_write exists solely for testing/mockability
   virtual int sslWriteImpl(SSL *ssl, const void *buf, int n) {
 
   // This virtual wrapper around SSL_write exists solely for testing/mockability
   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 {
   bool sessionResumptionAttempted_{false};
 
   std::unique_ptr<IOBuf> preReceivedData_;
   bool sessionResumptionAttempted_{false};
 
   std::unique_ptr<IOBuf> preReceivedData_;

+  // Whether or not to clear the err stack before invocation of another
+  // SSL method
+  bool clearOpenSSLErrors_{false};

 };
 
 } // namespace
 };
 
 } // namespace