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 d6345f71501d2acc3e20384943045d90ebdfa846..f30715d3577088092683bb0025724c7d2fc71a03 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -1277,10 +1277,22 @@ int SSLClientSocketOpenSSL::BufferSend(void) {
     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(
@@ -1322,10 +1334,20 @@ int SSLClientSocketOpenSSL::BufferRecv(void) {
   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) {
@@ -1367,6 +1389,11 @@ void SSLClientSocketOpenSSL::TransportWriteComplete(int result) {
     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)
@@ -1390,8 +1417,11 @@ int SSLClientSocketOpenSSL::TransportReadComplete(int result) {
     // 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);