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..548c349529621f0cfdd84ff4f83eb011718e4f44 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -34,6 +34,33 @@ namespace net {
 
 namespace {
 
+// This IOBuffer version is to be used with OpenSSL's non-copying interface,
+// to guarantee that the BIO object remains alive as long as the socket keeps
+// a reference to it's internal data buffer.
+//
+// |bio_buffer| must be retrieved from |bio| using the non-copying interface
+// BIO_nread0() or BIO_nwrite0(). The reference count of |bio| is increased to
+// make sure |bio| is not free'd if it is referenced from elsewhere.
+class WrappedBIOAndIOBuffer : public IOBuffer {
+ public:
+
+  explicit WrappedBIOAndIOBuffer(const char* bio_buffer, BIO* bio)

[Ryan Sleevi, 2014/04/30 21:33:32]

nit: delete newline on 46

+      : IOBuffer(const_cast<char*>(bio_buffer)), bio_(bio) {
+    CHECK(bio);
+
+    CRYPTO_add(&bio_->references, 1, CRYPTO_LOCK_BIO);
+  }
+
+ protected:
+  virtual ~WrappedBIOAndIOBuffer() {
+    data_ = NULL;
+    // Will count down the reference count, and free if the count reaches zero.
+    BIO_free_all(bio_);
+  }
+
+  BIO* bio_;
+};
+
 // Enable this to see logging for state machine state transitions.
 #if 0
 #define GotoState(s) do { DVLOG(2) << (void *)this << " " << __FUNCTION__ << \
@@ -1277,10 +1304,23 @@ 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.

[Ryan Sleevi, 2014/04/30 21:33:32]

Is this still necessary, given that BIO_nread0 with a NULL buf (second arg) will
return the size available for reading?

Is this even needed to check, now that we follow a non-copying pattern? If
available_read_bytes is 0, we can fall through, otherwise we can continue as
normal.

[haavardm, 2014/05/01 16:55:19]

The problem with letting 0 fall through is that calling  BIO_nread0 when there's
no data triggers the retry flag and the request for one byte. This happens
through the call to bio_read(bio, &dummy, 1) to avoid "code duplication " (see
bss_bio.c:303). That's a side effect that doesn't happen today and to me this
seems like something we want to avoid. 

On 2014/04/30 21:33:32, Ryan Sleevi wrote:

[Ryan Sleevi, 2014/05/08 22:59:28]

Yeah, I suppose the alternative would be to reset the retry flag, but I agree,
that seems less than ideal.

-    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);
+    // 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);
+    if (available_read_bytes <= 0)
+      return 0;
+
+    send_buffer_ = new DrainableIOBuffer(

[Ryan Sleevi, 2014/04/30 21:33:32]

Huh. I'm not really sure why we use a DrainableIOBuffer for the OpenSSL case (we
don't for NSS, which also uses a circular buffer).

[haavardm, 2014/05/01 16:55:19]

I believe the drainable buffer is needed to handle partial write.

It's useful to be able to consume data if TransportWriteComplete() gets called
with only partial data sent. According to the socket documentation this may
happen :"Note: data may be written partially."

The calls BIO_nread() and Consume() in TransportWriteComplete() keeps the read
and write indexes of transport_bio_ and send_buffer_  in sync.

[Ryan Sleevi, 2014/05/08 22:59:28]

Yeah, but why create a DrainableIOBuffer (and have custom logic for spinning the
loop), as opposed to using BIO_nread0 to get the available, and then once data
is written - partial or not - using BIO_nread to commit that many bytes
(1421-1422)

In the code prior to this, once we used BIO_read, we had committed to the BIO
that we'd write that much data, so we had to use a DrainableIOBuffer to keep
track of that. However, with the copy-less approach, I *think* it should be
sufficient to BIO_nread0 the maximum, and then, based on what the transport
wrote, just advance the circular buffer with the BIO_nread.

eg: assume we have 10 bytes in the circular buffer
BIO_ctrl_pending == 10
BIO_nread0(bio, buf) == 10
transport_->Write(..., 10) == 5 [only half the buffer was written]
BIO_nread(bio, buf, 5) // what to do with ret here - currently dropping to the
floor?

Which will then cause
BIO_ctrl_pending == 5
BIO_nread(bio, buf) == 5
transport_->Write(..., 5) == 5 [the remaining half]
BIO_nread(bio, buf, 5);

which will then cause
BIO_ctrl_pending == 0

The DrainableIOBuffer was our poor-man's way of simulating that.

[haavardm, 2014/05/09 07:23:41]

Yes, I had a look into NSS yesterday and saw how you do it there. In NSS (as
opposed to  OpenSSL standard copy interface) you are able to report how much you
have written.

I agree we can remove the this for non-copy interface since OpenSSL takes care
of the accounting. It will also simplify the code.

+        new WrappedBIOAndIOBuffer(bio_transport_buf, transport_bio_),
+        available_read_bytes);
   }
 
   int rv = transport_->socket()->Write(
@@ -1322,10 +1362,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 WrappedBIOAndIOBuffer(bio_transport_buf, transport_bio_);
   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 +1417,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 +1445,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);