Implement zero-copy SSL buffers.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 4e86c055932ed7517600625db0179d13ee720029..d7b028ccba0f7789ed056a37d3de489f252cc9d3 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -68,6 +68,9 @@ const int kNoPendingReadResult = 1;
 // 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);
 }
@@ -728,9 +731,20 @@ int SSLClientSocketOpenSSL::Init() {
   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_);
@@ -1509,20 +1523,20 @@ 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) {
@@ -1555,11 +1569,20 @@ int SSLClientSocketOpenSSL::BufferRecv(void) {
   // 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,
@@ -1574,7 +1597,6 @@ int SSLClientSocketOpenSSL::BufferRecv(void) {
 }
 
 void SSLClientSocketOpenSSL::BufferSendComplete(int result) {
-  transport_send_busy_ = false;
   TransportWriteComplete(result);
   OnSendComplete(result);
 }
@@ -1586,18 +1608,19 @@ void SSLClientSocketOpenSSL::BufferRecvComplete(int 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) {
@@ -1606,18 +1629,19 @@ int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   // 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;
 }