Repository: pyca/pyopenssl
Issue category: Performance Issue

Issue:
Poor performance on Connection.recv() with large values of bufsiz.
Discovered while investigating kennethreitz/requests#3729.

When allocating a buffer using CFFI's `new` method (as `Connection.recv` does via `_ffi.new("char[]", bufsiz)`, CFFI kindly zeroes that buffer for us. That means this call has a performance cost that is linear in `bufsiz`. This can lead to nasty negative behaviour if really large values of `bufsiz` are used, e.g. 1GB. 

We don't need a zeroed buffer for this: `SSL_read` returns a length, so we know exactly how many bytes to copy out. Unfortunately though, CFFI doesn't provide any method to obtain a non-zeroed buffer, at least not as far as I can see.

Instead, a coping method might be to silently clamp the maximum value of `bufsiz` to the return value of `self._socket.getsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF)`. This is the size of the socket receive buffer, and therefore represents the maximum amount of data the kernel can possibly return in one `recv` call. On my OS X machine, this is ~129 kB by default. An alternative option is to clamp to 16kB, the maximum TLS record size, or to clamp to the nearest multiple of 16kB near `SO_RCVBUF`. Either of these will greatly shrink the allocations. Finally, we can just pick an arbitrary value: a few hundred KB is probably safe.

I should note, however, that this coping method *still* imposes a substantial cost when downloading data that is not incurred by the standard library's `ssl` module. Specifically, CFFI will require that we zero at least as many bytes as the size of the download. This is *all* wasted effort, and for multi-gigabyte files represents really quite a substantial waste of time. It would be best to do both of these things: to shrink the size of our allocations *and* to try to avoid zeroing data when we don't need to.


Hints:
So, Armin has pointed out that this actually already exists: `ffi.new_allocator(should_clear_after_alloc=False)("char[]", bufsiz)` should be a solution to the zeroing out of memory. I'm going to investigate it now.
Excellently, that does work. I'll start working on a PR for this tomorrow.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 19cebf1e8..56c3c74c8 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -25,6 +25,10 @@ Changes:
 
 - Added ``OpenSSL.X509Store.set_time()`` to set a custom verification time when verifying certificate chains.
   `#567 <https://github.com/pyca/pyopenssl/pull/567>`_
+- Changed the ``SSL`` module's memory allocation policy to avoid zeroing memory it allocates when unnecessary.
+  This reduces CPU usage and memory allocation time by an amount proportional to the size of the allocation.
+  For applications that process a lot of TLS data or that use very lage allocations this can provide considerable performance improvements.
+  `#578 <https://github.com/pyca/pyopenssl/pull/578>`_
 
 
 ----
diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py
index f92c06428..63a0b7e85 100644
--- a/src/OpenSSL/SSL.py
+++ b/src/OpenSSL/SSL.py
@@ -18,6 +18,7 @@
     native as _native,
     path_string as _path_string,
     text_to_bytes_and_warn as _text_to_bytes_and_warn,
+    no_zero_allocator as _no_zero_allocator,
 )
 
 from OpenSSL.crypto import (
@@ -1297,7 +1298,7 @@ def recv(self, bufsiz, flags=None):
             all other flags are ignored.
         :return: The string read from the Connection
         """
-        buf = _ffi.new("char[]", bufsiz)
+        buf = _no_zero_allocator("char[]", bufsiz)
         if flags is not None and flags & socket.MSG_PEEK:
             result = _lib.SSL_peek(self._ssl, buf, bufsiz)
         else:
@@ -1328,7 +1329,7 @@ def recv_into(self, buffer, nbytes=None, flags=None):
         # We need to create a temporary buffer. This is annoying, it would be
         # better if we could pass memoryviews straight into the SSL_read call,
         # but right now we can't. Revisit this if CFFI gets that ability.
-        buf = _ffi.new("char[]", nbytes)
+        buf = _no_zero_allocator("char[]", nbytes)
         if flags is not None and flags & socket.MSG_PEEK:
             result = _lib.SSL_peek(self._ssl, buf, nbytes)
         else:
@@ -1379,7 +1380,7 @@ def bio_read(self, bufsiz):
         if not isinstance(bufsiz, integer_types):
             raise TypeError("bufsiz must be an integer")
 
-        buf = _ffi.new("char[]", bufsiz)
+        buf = _no_zero_allocator("char[]", bufsiz)
         result = _lib.BIO_read(self._from_ssl, buf, bufsiz)
         if result <= 0:
             self._handle_bio_errors(self._from_ssl, result)
@@ -1616,7 +1617,7 @@ def server_random(self):
             return None
         length = _lib.SSL_get_server_random(self._ssl, _ffi.NULL, 0)
         assert length > 0
-        outp = _ffi.new("unsigned char[]", length)
+        outp = _no_zero_allocator("unsigned char[]", length)
         _lib.SSL_get_server_random(self._ssl, outp, length)
         return _ffi.buffer(outp, length)[:]
 
@@ -1632,7 +1633,7 @@ def client_random(self):
 
         length = _lib.SSL_get_client_random(self._ssl, _ffi.NULL, 0)
         assert length > 0
-        outp = _ffi.new("unsigned char[]", length)
+        outp = _no_zero_allocator("unsigned char[]", length)
         _lib.SSL_get_client_random(self._ssl, outp, length)
         return _ffi.buffer(outp, length)[:]
 
@@ -1648,7 +1649,7 @@ def master_key(self):
 
         length = _lib.SSL_SESSION_get_master_key(session, _ffi.NULL, 0)
         assert length > 0
-        outp = _ffi.new("unsigned char[]", length)
+        outp = _no_zero_allocator("unsigned char[]", length)
         _lib.SSL_SESSION_get_master_key(session, outp, length)
         return _ffi.buffer(outp, length)[:]
 
@@ -1788,7 +1789,7 @@ def _get_finished_message(self, function):
             # No Finished message so far.
             return None
 
-        buf = _ffi.new("char[]", size)
+        buf = _no_zero_allocator("char[]", size)
         function(self._ssl, buf, size)
         return _ffi.buffer(buf, size)[:]
 
diff --git a/src/OpenSSL/_util.py b/src/OpenSSL/_util.py
index 48bcbf561..cf8b8880a 100644
--- a/src/OpenSSL/_util.py
+++ b/src/OpenSSL/_util.py
@@ -12,6 +12,12 @@
 lib = binding.lib
 
 
+# This is a special CFFI allocator that does not bother to zero its memory
+# after allocation. This has vastly better performance on large allocations and
+# so should be used whenever we don't need the memory zeroed out.
+no_zero_allocator = ffi.new_allocator(should_clear_after_alloc=False)
+
+
 def text(charp):
     """
     Get a native string type representing of the given CFFI ``char*`` object.


Test patch: