Make OpenSSLSession refcount updates part of the critical section

Summary: Prevent a race condition where a session's reference count can be decremented to 0 (causing the session to be freed) by one thread before another thread can increment the reference count.

Reviewed By: mingtaoy

Differential Revision: D26614966

fbshipit-source-id: 6c55321becfc1a3467f57c692065680a32ac21f3

folly/ssl/detail/OpenSSLSession.cpp

@@ -16,8 +16,8 @@
 
 #include <folly/ssl/detail/OpenSSLSession.h>
 
-#include <atomic>
-
+#include <folly/SharedMutex.h>
+#include <folly/Synchronized.h>
 #include <folly/portability/OpenSSL.h>
 #include <folly/ssl/OpenSSLPtrTypes.h>
 
@@ -25,26 +25,24 @@ namespace folly {
 namespace ssl {
 namespace detail {
 
-OpenSSLSession::~OpenSSLSession() {
-  SSL_SESSION* session = activeSession_.load();
-  if (session) {
-    SSL_SESSION_free(session);
-  }
-}
-
 void OpenSSLSession::setActiveSession(SSLSessionUniquePtr s) {
-  SSL_SESSION* oldSession = activeSession_.exchange(s.release());
-  if (oldSession) {
-    SSL_SESSION_free(oldSession);
-  }
+  // OpenSSLSession is typically shared as a std::shared_ptr<SSLSession>,
+  // and setActiveSession() may be invoked in mulitple threads. Consequently,
+  // changing the `activeSession_ pointer needs to be synchronized,
+  // such that readers are able to fully acquire a reference count in
+  // getActiveSession().
+
+  activeSession_.withWLock([&](auto& sessionPtr) { sessionPtr.swap(s); });
 }
 
 SSLSessionUniquePtr OpenSSLSession::getActiveSession() {
-  SSL_SESSION* session = activeSession_.load();
-  if (session) {
-    SSL_SESSION_up_ref(session);
-  }
-  return SSLSessionUniquePtr(session);
+  return activeSession_.withRLock([](auto& sessionPtr) {
+    SSL_SESSION* session = sessionPtr.get();
+    if (session) {
+      SSL_SESSION_up_ref(session);
+    }
+    return SSLSessionUniquePtr(session);
+  });
 }
 
 } // namespace detail

folly/ssl/detail/OpenSSLSession.h

@@ -16,8 +16,8 @@
 
 #pragma once
 
-#include <atomic>
-
+#include <folly/SharedMutex.h>
+#include <folly/Synchronized.h>
 #include <folly/portability/OpenSSL.h>
 #include <folly/ssl/OpenSSLPtrTypes.h>
 #include <folly/ssl/SSLSession.h>
@@ -38,7 +38,7 @@ namespace detail {
 
 class OpenSSLSession : public SSLSession {
  public:
-  ~OpenSSLSession();
+  ~OpenSSLSession() = default;
 
   /**
    * Set the underlying SSL session. Any previously held session
@@ -52,7 +52,7 @@ class OpenSSLSession : public SSLSession {
   SSLSessionUniquePtr getActiveSession();
 
  private:
-  std::atomic<SSL_SESSION*> activeSession_;
+  folly::Synchronized<SSLSessionUniquePtr, folly::SharedMutex> activeSession_;
 };
 
 } // namespace detail