Implement zero-copy SSL buffers.

net/socket/ssl_client_socket_openssl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <errno.h>
@@ skipping 50 matching lines @@
 #endif
 
 // This constant can be any non-negative/non-zero value (eg: it does not
 // overlap with any value of the net::Error range, including net::OK).
 const int kNoPendingReadResult = 1;
 
 // If a client doesn't have a list of protocols that it supports, but
 // the server supports NPN, choosing "http/1.1" is the best answer.
 const char kDefaultSupportedNPNProtocol[] = "http/1.1";
 
+// Default sizes of the internal BoringSSL buffers.

[agl, 2014/12/17 19:12:22]

// Default size of the internal BoringSSL buffers.

[haavardm, 2014/12/18 12:18:07]

Done.

+const int KDefaultOpenSSLBufferSize = 17 * 1024;
+
 void FreeX509Stack(STACK_OF(X509)* ptr) {
   sk_X509_pop_free(ptr, X509_free);
 }
 
 typedef crypto::ScopedOpenSSL<X509, X509_free>::Type ScopedX509;
 typedef crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>::Type
     ScopedX509Stack;
 
 #if OPENSSL_VERSION_NUMBER < 0x1000103fL
 // This method doesn't seem to have made it into the OpenSSL headers.
@@ skipping 640 matching lines @@
 
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
   // Set an OpenSSL callback to monitor this SSL*'s connection.
   SSL_set_info_callback(ssl_, &InfoCallback);
 
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
       ssl_, GetSessionCacheKey());
 
+  send_buffer_ = new GrowableIOBuffer();
+  send_buffer_->SetCapacity(KDefaultOpenSSLBufferSize);
+  recv_buffer_ = new GrowableIOBuffer();
+  recv_buffer_->SetCapacity(KDefaultOpenSSLBufferSize);
+
   BIO* ssl_bio = NULL;
-  // 0 => use default buffer sizes.
-  if (!BIO_new_bio_pair(&ssl_bio, 0, &transport_bio_, 0))
+
+  // Let |SSLClientSocketOpenSSL| have ownership over the bio buffers to keep

[agl, 2014/12/17 19:12:22]

// SSLClientSocketOpenSSL retains ownership of the BIO buffers.

[haavardm, 2014/12/18 12:18:07]

Done.

+  // the IOBuffers user contract.
+  if (!BIO_new_bio_pair_external_buf(
+          &ssl_bio, send_buffer_->capacity(),
+          reinterpret_cast<uint8_t*>(send_buffer_->data()), &transport_bio_,
+          recv_buffer_->capacity(),
+          reinterpret_cast<uint8_t*>(recv_buffer_->data())))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
   // Install a callback on OpenSSL's end to plumb transport errors through.
   BIO_set_callback(ssl_bio, BIOCallback);
   BIO_set_callback_arg(ssl_bio, reinterpret_cast<char*>(this));
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
@@ skipping 758 matching lines @@
         NetLog::TYPE_SSL_WRITE_ERROR,
         CreateNetLogOpenSSLErrorCallback(net_error, ssl_error, error_info));
   }
   return net_error;
 }
 
 int SSLClientSocketOpenSSL::BufferSend(void) {
   if (transport_send_busy_)
     return ERR_IO_PENDING;
 
-  if (!send_buffer_.get()) {
-    // Get a fresh send buffer out of the send BIO.
-    size_t max_read = BIO_pending(transport_bio_);
-    if (!max_read)
-      return 0;  // Nothing pending in the OpenSSL write BIO.
-    send_buffer_ = new DrainableIOBuffer(new IOBuffer(max_read), max_read);
-    int read_bytes = BIO_read(transport_bio_, send_buffer_->data(), max_read);
-    DCHECK_GT(read_bytes, 0);
-    CHECK_EQ(static_cast<int>(max_read), read_bytes);
-  }
+  size_t buffer_read_offset;
+  uint8_t* read_buf;
+  size_t max_read;
+  int status = BIO_zero_copy_get_read_buf(transport_bio_, &read_buf,
+                                          &buffer_read_offset, &max_read);
+  DCHECK(status == 1);  // Should never fail.

[agl, 2014/12/17 22:29:56]

DCHECK_EQ

[haavardm, 2014/12/18 12:18:07]

Done.

+  if (!max_read)
+    return 0;  // Nothing pending in the OpenSSL write BIO.
+  CHECK(read_buf == reinterpret_cast<uint8_t*>(send_buffer_->data()));

[agl, 2014/12/17 22:29:56]

CHECK_EQ

[haavardm, 2014/12/18 12:18:07]

Done.

+  CHECK(buffer_read_offset < static_cast<size_t>(send_buffer_->capacity()));

[agl, 2014/12/17 22:29:56]

CHECK_LT

[haavardm, 2014/12/18 12:18:07]

Done.

+  send_buffer_->set_offset(buffer_read_offset);
 
   int rv = transport_->socket()->Write(
-      send_buffer_.get(),
-      send_buffer_->BytesRemaining(),
+      send_buffer_.get(), max_read,
       base::Bind(&SSLClientSocketOpenSSL::BufferSendComplete,
                  base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
     transport_send_busy_ = true;
   } else {
     TransportWriteComplete(rv);
   }
   return rv;
 }
 
@@ skipping 11 matching lines @@
     return ERR_IO_PENDING;
   }
 
   // Known Issue: While only reading |requested| data is the more correct
   // implementation, it has the downside of resulting in frequent reads:
   // One read for the SSL record header (~5 bytes) and one read for the SSL
   // record body. Rather than issuing these reads to the underlying socket
   // (and constantly allocating new IOBuffers), a single Read() request to
   // fill |transport_bio_| is issued. As long as an SSL client socket cannot
   // be gracefully shutdown (via SSL close alerts) and re-used for non-SSL
   // traffic, this over-subscribed Read()ing will not cause issues.

[Ryan Sleevi, 2014/12/17 22:57:10]

Is this comment still applicable/accurate? Does this still fill
|transport_bio_|, or does max_write indicate how much they requested from the
BIO?

[haavardm, 2014/12/18 12:18:07]

I've done no changes to that logic, so max_write is still maximum number of
bytes available for write in |transport_bio|. 

-  size_t max_write = BIO_ctrl_get_write_guarantee(transport_bio_);
+
+  size_t buffer_write_offset;
+  uint8_t* write_buf;
+  size_t max_write;
+  int status = BIO_zero_copy_get_write_buf(transport_bio_, &write_buf,
+                                           &buffer_write_offset, &max_write);
+  DCHECK(status == 1);  // Should never fail.

[agl, 2014/12/17 22:29:56]

DCHECK_EQ

   if (!max_write)
     return ERR_IO_PENDING;
 
-  recv_buffer_ = new IOBuffer(max_write);
+  CHECK(write_buf == reinterpret_cast<uint8_t*>(recv_buffer_->data()));

[agl, 2014/12/17 22:29:56]

CHECK_EQ

+  CHECK(buffer_write_offset < static_cast<size_t>(recv_buffer_->capacity()));

[agl, 2014/12/17 22:29:56]

CHECK_LT

+
+  recv_buffer_->set_offset(buffer_write_offset);
   int rv = transport_->socket()->Read(
       recv_buffer_.get(),
       max_write,
       base::Bind(&SSLClientSocketOpenSSL::BufferRecvComplete,
                  base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
     transport_recv_busy_ = true;
   } else {
     rv = TransportReadComplete(rv);
   }
   return rv;
 }
 
 void SSLClientSocketOpenSSL::BufferSendComplete(int result) {
-  transport_send_busy_ = false;
   TransportWriteComplete(result);
   OnSendComplete(result);
 }
 
 void SSLClientSocketOpenSSL::BufferRecvComplete(int result) {
   result = TransportReadComplete(result);
   OnRecvComplete(result);
 }
 
 void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
+  int bytes_written = 0;
   if (result < 0) {
     // Record the error. Save it to be reported in a future read or write on
     // transport_bio_'s peer.
     transport_write_error_ = result;
-    send_buffer_ = NULL;
   } else {
-    DCHECK(send_buffer_.get());
-    send_buffer_->DidConsume(result);
-    DCHECK_GE(send_buffer_->BytesRemaining(), 0);
-    if (send_buffer_->BytesRemaining() <= 0)
-      send_buffer_ = NULL;
+    bytes_written = result;
   }
+  DCHECK_GE(send_buffer_->RemainingCapacity(), bytes_written);
+  int ret = BIO_zero_copy_get_read_buf_done(transport_bio_, bytes_written);

[Ryan Sleevi, 2014/12/17 22:57:10]

What's the contract from BoringSSL if the zero-copy buffers if bytes_written was
< max_read from line 1530?

Will it handle this appropriately? I would assume so, but wanting to confirm.

[haavardm, 2014/12/18 12:18:07]

Yes, BoringSSL will handle this case. BIO_zero_copy_get_read_buf_done will
adjust the internal offset according to bytes_written and transport_bio is
unlocked so that BIO_zero_copy_get_read_buf can be called again.

To be completely sure I added a few lines to
src/third_party/boringssl/src/crypto/bio/bio_test.c  here locally (I had only
tested partially write using this API). I might upstream.

Note that every call to BIO_zero_copy_get_read_buf_done has to be preceded by a
call to  BIO_zero_copy_get_read_buf. Thus the assumption here is that
TransportWriteComplete is never called more than once after a single to
socket->Write.

+  DCHECK_EQ(1, ret);
+  send_buffer_->set_offset(0);

[agl, 2014/12/17 22:29:56]

Is this set_offset needed? It seems that it would be better done closer to the
Read and Write calls, even if that meant that it was duplicated.

[haavardm, 2014/12/18 12:18:07]

It is not really needed since the offset is always set before the buffer is
used. I just found it clean to reset the offset when the buffer goes idle. I
think I'll rather remove completely instead as it builds up a expectation that
is has to be reset to 0.

[haavardm, 2014/12/18 12:18:07]

Done.

+  transport_send_busy_ = false;
 }
 
 int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   // If an EOF, canonicalize to ERR_CONNECTION_CLOSED here so MapOpenSSLError
   // does not report success.
   if (result == 0)
     result = ERR_CONNECTION_CLOSED;
+  int bytes_read = 0;
   if (result < 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
     // Received an error. Save it to be reported in a future read on
     // transport_bio_'s peer.
     transport_read_error_ = result;
   } else {
-    DCHECK(recv_buffer_.get());
-    int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
-    // A write into a memory BIO should always succeed.
-    DCHECK_EQ(result, ret);
+    bytes_read = result;
   }
-  recv_buffer_ = NULL;
+  DCHECK_GE(recv_buffer_->RemainingCapacity(), bytes_read);
+  int ret = BIO_zero_copy_get_write_buf_done(transport_bio_, bytes_read);
+  DCHECK_EQ(1, ret);
+  recv_buffer_->set_offset(0);

[agl, 2014/12/17 22:29:56]

Ditto.

[haavardm, 2014/12/18 12:18:07]

Done.

   transport_recv_busy_ = false;
   return result;
 }
 
 int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/424386 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "424386 SSLClientSocketOpenSSL::ClientCertRequestCallback"));
 
@@ skipping 272 matching lines @@
                                             ct::SCT_STATUS_LOG_UNKNOWN));
   }
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net