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 <openssl/err.h>
@@ skipping 1259 matching lines @@
 
 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_ctrl_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);
+
+    char* bio_transport_buf;
+    // Get the internal buffer and number of bytes available for non-copying
+    // read.
+    // BIO_ctrl_pending() is still called above. Calling BIO_nread0() when
+    // there is no data triggers a call to BIO_nread() to read one byte. This
+    // has side effect that the retry flag is set.
+    int available_read_bytes = BIO_nread0(transport_bio_, &bio_transport_buf);

[Ryan Sleevi, 2014/04/29 18:57:02]

Note the comments in bss_bio.c

WARNING: The non-copying interface is largely untested as of yet and may contain
bugs.

A little nervous, but hopefully unit tests will cover this.

[haavardm, 2014/04/29 20:52:56]

Yes I saw that too and agree it's a bit risky, especially in the light of recent
events regarding buffer handling in OpenSSL. Decided to have a go anyway. The
API is not  much code, so it should be possible get in control of it and
understand the subtleness of it. The results of my analysis are in the comments
here. 
The unit tests runs fine on linux, but I would like more tests with different 
(especially smaller) buffer sizes in the tcp layer.

+    // BIO_nread0() might report less than BIO_ctrl_pending() due to the buffer
+    // offset and no ring buffer wrap-around for non-copying interface.
+    CHECK_GE(static_cast<int>(max_read), available_read_bytes);

[Ryan Sleevi, 2014/04/29 18:57:02]

Two things:
1) I'm a little nervous about this CHECK. A CHECK here would be in ensuring that
OpenSSL doesn't violate it's contract, but your comment doesn't suggest it's
that strong a guarantee from OpenSSL

2) I'm always nervous around size_t being cast to an int. That said, OpenSSL's
API is terribad here (with respect to proper integer types), but I'm sure you
can appreciate the concern.

[haavardm, 2014/04/29 20:52:56]

Normally, if the read/write index is in the middle of the buffer, while there
are free bytes in the beginning (end < index), then BIO_ctrl_pending()
calculates the total number of free bytes including the chunk from start.
BIO_read/BIO_write is able to wrap around and use all of the bytes when copying.
However, this wrap around is not possible when the Socket object writes directly
into the buffer. That is why the actual available bytes for non-copying
interface might be less. I I remember correctly, the same issue exists for NSS's
non-copying interface.

According to this, the contract is that max > available_read_bytes.
Absolutely. I do think it's safe in this case though. 

+    if (available_read_bytes <= 0)
+      return 0;
+
+    send_buffer_ = new DrainableIOBuffer(new WrappedIOBuffer(bio_transport_buf),
+                                         available_read_bytes);

[Ryan Sleevi, 2014/04/29 18:57:02]

BUG: So, the contract of IOBuffer is that, although it's ref-counted, the
pointer must remain valid for the lifetime of the object, and it's legal for the
consumer (in this case, the transport socket) to hold onto it beyond the
object's destruction.

A real world example of this is TCPClientSocketWin, which has
TCPClientSocketWin::Core, which outlives the *SocketWin out of necessity, due to
IO Completion Ports' behaviour that requires the buffers to be valid for the
duration of the IOCP call.

This is unsafe, because the buffer returned by BIO_nread0() only remains valid
for the duration of transport_bio_, which is tied to the lifetime of this
object.

Thus you have a situation where SSLClientSocketOpenSSL is dtor'd, the
transport_bio_ is released, transport_ is freed, but there is still an object
that holds on to the IOBuffer and expects the buffer to remain valid (until the
IOCP completes).

We likely need to create a new IOBuffer type that can ensure that the
transport_bio_ remains valid for as long as there are one or more IOBuffer's
that still hold onto it.

This also means that we need to consider the transfer semantics of the BIO* and
the SSL* - if ownership of the BIO* is transferred to the SSL*, then we really
have to keep the SSL* object alive as long as there are outstanding IOBuffers
pointing into the BIO* buffer.

[haavardm, 2014/04/29 20:52:56]

Ah, right. I knew about the contract for IOBuffer but failed to realize that the
OS could hold on to the buffer until read/write was done, even after destruction
of the socket. I'm afraid my experience with Windows socket API is a bit
limited.

The documentation of Socket says:
"If the operation is not completed immediately, the socket acquires a reference
to the provided buffer until the callback is invoked or the socket is closed."

I understood this as the socket would release the valid buffer requirement when
the Socket object is closed/destructed by the owner. 

   }
 
   int rv = transport_->socket()->Write(
       send_buffer_.get(),
       send_buffer_->BytesRemaining(),
       base::Bind(&SSLClientSocketOpenSSL::BufferSendComplete,
                  base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
     transport_send_busy_ = true;
   } else {
@@ skipping 21 matching lines @@
   // 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.
   size_t max_write = BIO_ctrl_get_write_guarantee(transport_bio_);
   if (!max_write)
     return ERR_IO_PENDING;
 
-  recv_buffer_ = new IOBuffer(max_write);
+  char* bio_transport_buf;
+  // Get the internal buffer and number of bytes available for non-copying
+  // write.
+  int available_write_bytes = BIO_nwrite0(transport_bio_, &bio_transport_buf);
+  // BIO_nwrite0() might report less than BIO_ctrl_get_write_guarantee() due to
+  // the buffer offset and no ring buffer wrap-around for non-copying interface.
+  DCHECK_GE(static_cast<int>(max_write), available_write_bytes);
+  if (available_write_bytes <= 0)
+    return ERR_IO_PENDING;
+
+  recv_buffer_ = new WrappedIOBuffer(bio_transport_buf);
   int rv = transport_->socket()->Read(
       recv_buffer_.get(),
-      max_write,
+      available_write_bytes,
       base::Bind(&SSLClientSocketOpenSSL::BufferRecvComplete,
                  base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
     transport_recv_busy_ = true;
   } else {
     rv = TransportReadComplete(rv);
   }
   return rv;
 }
 
@@ skipping 21 matching lines @@
 
     // Match the fix for http://crbug.com/249848 in NSS by erroring future reads
     // from the socket after a write error.
     //
     // TODO(davidben): Avoid having read and write ends interact this way.
     transport_write_error_ = result;
     (void)BIO_shutdown_wr(transport_bio_);
     send_buffer_ = NULL;
   } else {
     DCHECK(send_buffer_.get());
+    char* bio_transport_buf;
+    // Advance the buffer index.
+    int ret = BIO_nread(transport_bio_, &bio_transport_buf, result);
+    CHECK_EQ(ret, result);
+    CHECK_EQ(bio_transport_buf, send_buffer_->data());
     send_buffer_->DidConsume(result);
     DCHECK_GE(send_buffer_->BytesRemaining(), 0);
     if (send_buffer_->BytesRemaining() <= 0)
       send_buffer_ = NULL;
   }
 }
 
 int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
   DCHECK(ERR_IO_PENDING != result);
   if (result <= 0) {
     DVLOG(1) << "TransportReadComplete result " << result;
     // Received 0 (end of file) or an error. Either way, bubble it up to the
     // SSL layer via the BIO. TODO(joth): consider stashing the error code, to
     // relay up to the SSL socket client (i.e. via DoReadCallback).
     if (result == 0)
       transport_recv_eof_ = true;
     (void)BIO_shutdown_wr(transport_bio_);
   } else if (transport_write_error_ < 0) {
     // Mirror transport write errors as read failures; transport_bio_ has been
     // shut down by TransportWriteComplete, so the BIO_write will fail, failing
     // the CHECK. http://crbug.com/335557.
     result = transport_write_error_;
   } else {
-    DCHECK(recv_buffer_.get());
-    int ret = BIO_write(transport_bio_, recv_buffer_->data(), result);
+    char* bio_transport_buf;
+    // Set the amount of data read into buffer.
+    int ret = BIO_nwrite(transport_bio_, &bio_transport_buf, result);
+    CHECK_EQ(ret, result);
+    CHECK_EQ(bio_transport_buf, recv_buffer_->data());
     // A write into a memory BIO should always succeed.
     // Force values on the stack for http://crbug.com/335557
     base::debug::Alias(&result);
     base::debug::Alias(&ret);
     CHECK_EQ(result, ret);
   }
   recv_buffer_ = NULL;
   transport_recv_busy_ = false;
   return result;
 }
@@ skipping 160 matching lines @@
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
   return SSL_TLSEXT_ERR_OK;
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net