add an option to clear error before calling ssl methods
authorSubodh Iyengar <subodh@fb.com>
Tue, 10 Jan 2017 23:51:33 +0000 (15:51 -0800)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Wed, 11 Jan 2017 00:02:57 +0000 (16:02 -0800)
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

folly/io/async/AsyncSSLSocket.cpp
folly/io/async/AsyncSSLSocket.h

index 7910bd0..cb4b96d 100644 (file)
@@ -22,8 +22,8 @@
 #include <boost/noncopyable.hpp>
 #include <errno.h>
 #include <fcntl.h>
-#include <openssl/err.h>
 #include <openssl/asn1.h>
+#include <openssl/err.h>
 #include <openssl/ssl.h>
 #include <sys/types.h>
 #include <chrono>
@@ -1030,6 +1030,7 @@ AsyncSSLSocket::handleAccept() noexcept {
     SSL_set_msg_callback_arg(ssl_, this);
   }
 
+  clearOpenSSLErrors();
   int ret = SSL_accept(ssl_);
   if (ret <= 0) {
     int sslError;
@@ -1079,6 +1080,18 @@ AsyncSSLSocket::handleAccept() noexcept {
   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
@@ -1094,6 +1107,7 @@ AsyncSSLSocket::handleConnect() noexcept {
       sslState_ == STATE_CONNECTING);
   assert(ssl_);
 
+  clearOpenSSLErrors();
   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);
   }
 
+  clearOpenSSLErrors();
   int bytes = 0;
   if (!isBufferMovable_) {
     bytes = SSL_read(ssl_, *buf, int(*buflen));
index 01ff7bd..e7d499e 100644 (file)
@@ -665,9 +665,20 @@ class AsyncSSLSocket : public virtual AsyncSocket {
     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();
+  void clearOpenSSLErrors();
 
  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) {
+    clearOpenSSLErrors();
     return SSL_write(ssl, buf, n);
   }
 
@@ -822,6 +834,9 @@ class AsyncSSLSocket : public virtual AsyncSocket {
   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