From 3a8c98ea2b9d70762b562e0c4b8aff7fbfd683a5 Mon Sep 17 00:00:00 2001
From: Subodh Iyengar <subodh@fb.com>
Date: Tue, 10 Jan 2017 15:51:33 -0800
Subject: [PATCH] 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
---
 folly/io/async/AsyncSSLSocket.cpp | 17 ++++++++++++++++-
 folly/io/async/AsyncSSLSocket.h   | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/folly/io/async/AsyncSSLSocket.cpp b/folly/io/async/AsyncSSLSocket.cpp
index 7910bd02..cb4b96d2 100644
--- 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 <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));
diff --git a/folly/io/async/AsyncSSLSocket.h b/folly/io/async/AsyncSSLSocket.h
index 01ff7bd2..e7d499e9 100644
--- a/folly/io/async/AsyncSSLSocket.h
+++ b/folly/io/async/AsyncSSLSocket.h
@@ -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
-- 
2.34.1