ext/openssl: make OpenSSL::SSL::SSLSocket non-reusable

Fix the segmentation fault due to use after free of SSL object.

Don't free the SSL object in SSLSocket#stop and leave it in RTypedData.

OpenSSL::SSL::SSLSocket#stop (and the wrapper method #sysclose) was
documented as "Stops the SSL connection and prepares it for another
connection" and this patch changes it: once the SSLSocket stopped, it
can no longer hold a new SSL connection.

The method used to free the SSL object immediately after SSL_shutdown(),
but it is problematic. Since some methods, such as SSLSocket#connect
release the GVL while waiting the peer, there is a good chance that the
SSL object is freed from another thread and this leads to a segmentation
fault.

3 files changed, 29 insertions, 5 deletions

diff --git a/ext/openssl/lib/openssl/ssl.rb b/ext/openssl/lib/openssl/ssl.rb
index 57519f2c21..9893757011 100644
--- a/ext/openssl/lib/openssl/ssl.rb
+++ b/ext/openssl/lib/openssl/ssl.rb
@@ -290,7 +290,10 @@ module OpenSSL
       # call-seq:
       #    ssl.sysclose => nil
       #
-      # Shuts down the SSL connection and prepares it for another connection.
+      # Sends "close notify" to the peer and tries to shut down the SSL
+      # connection gracefully.
+      #
+      # If sync_close is set to +true+, the underlying IO is also closed.
       def sysclose
         return if closed?
         stop
diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c
index 96c7990046..416fa91cae 100644
--- a/ext/openssl/ossl_ssl.c
+++ b/ext/openssl/ossl_ssl.c
@@ -1598,7 +1598,8 @@ ossl_ssl_write_nonblock(int argc, VALUE *argv, VALUE self)
  * call-seq:
  *    ssl.stop => nil
  *
- * Stops the SSL connection and prepares it for another connection.
+ * Sends "close notify" to the peer and tries to shut down the SSL connection
+ * gracefully.
  */
 static VALUE
 ossl_ssl_stop(VALUE self)
@@ -1607,14 +1608,12 @@ ossl_ssl_stop(VALUE self)
 
     /* ossl_ssl_data_get_struct() is not usable here because it may return
      * from this function; */
-
     GetSSL(self, ssl);
 
     if (ssl) {
+	/* the SSL object will be freed by GC */
 	ossl_ssl_shutdown(ssl);
-	SSL_free(ssl);
     }
-    DATA_PTR(self) = NULL;
 
     return Qnil;
 }
diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb
index 6e9078dace..d7b996d662 100644
--- a/test/openssl/test_ssl.rb
+++ b/test/openssl/test_ssl.rb
@@ -1169,6 +1169,28 @@ end
     }
   end
 
+  def test_close_and_socket_close_while_connecting
+    # test it doesn't cause a segmentation fault
+    ctx = OpenSSL::SSL::SSLContext.new
+    ctx.ciphers = "aNULL"
+
+    sock1, sock2 = socketpair
+    ssl1 = OpenSSL::SSL::SSLSocket.new(sock1, ctx)
+    ssl2 = OpenSSL::SSL::SSLSocket.new(sock2, ctx)
+
+    t = Thread.new { ssl1.connect }
+    ssl2.accept
+
+    ssl1.close
+    sock1.close
+    t.value rescue nil
+  ensure
+    ssl1.close if ssl1
+    ssl2.close if ssl2
+    sock1.close if sock1
+    sock2.close if sock2
+  end
+
   def test_get_ephemeral_key
     return unless OpenSSL::SSL::SSLSocket.method_defined?(:tmp_key)
     pkey = OpenSSL::PKey