Fix SSLSocketAdapter to handle asynchronous writes appropriately.

remoting/jingle_glue/ssl_socket_adapter.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.
 
 #include "remoting/jingle_glue/ssl_socket_adapter.h"
 
 #include "base/base64.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
 #include "jingle/glue/utils.h"
 #include "net/base/address_list.h"
 #include "net/base/cert_verifier.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
 #include "net/base/ssl_config_service.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/url_request/url_request_context.h"
 
 namespace remoting {
 
 SSLSocketAdapter* SSLSocketAdapter::Create(AsyncSocket* socket) {
   return new SSLSocketAdapter(socket);
 }
 
 SSLSocketAdapter::SSLSocketAdapter(AsyncSocket* socket)
     : SSLAdapter(socket),
       ignore_bad_cert_(false),
       cert_verifier_(net::CertVerifier::CreateDefault()),
       ssl_state_(SSLSTATE_NONE),
-      read_state_(IOSTATE_NONE),
-      write_state_(IOSTATE_NONE),
-      data_transferred_(0) {
+      read_pending_(false),
+      write_pending_(false) {
   transport_socket_ = new TransportSocket(socket, this);
 }
 
 SSLSocketAdapter::~SSLSocketAdapter() {
 }
 
 int SSLSocketAdapter::StartSSL(const char* hostname, bool restartable) {
   DCHECK(!restartable);
   hostname_ = hostname;
 
@@ skipping 33 matching lines @@
 
   if (result == net::ERR_IO_PENDING || result == net::OK) {
     return 0;
   } else {
     LOG(ERROR) << "Could not start SSL: " << net::ErrorToString(result);
     return result;
   }
 }
 
 int SSLSocketAdapter::Send(const void* buf, size_t len) {
+  if (ssl_state_ == SSLSTATE_ERROR) {
+    SetError(EINVAL);
+    return -1;
+  }
+
   if (ssl_state_ != SSLSTATE_CONNECTED) {
+    // Propagate the call to underlying socket if SSL is not connected yet.

[Wez, 2012/06/13 00:48:57]

This explains what you're doing, but not _why_? Why is it appropriate to
propagate the call to the underlying socket? Surely that means sending
unencrypted data? When would we want to do that? Shouldn't we buffer the data
until SSL is connected, or return EWOULDBLOCK?

[Sergey Ulanov, 2012/06/13 01:30:31]

XMPP connections are not encrypted until <starttls> command is received, and the
XMPP stack needs to be able to send/receive unencrypted data until then. Once
connection is upgraded to TLS the upper layer calls StartSSL() that creates SSL
layer. Updated the comment. Also changed condition to ssl_state_ ==
SSLSTATE_NONE, as sending any data here when we are WAIT state would be
incorrect.

     return AsyncSocketAdapter::Send(buf, len);
-  } else {
-    scoped_refptr<net::IOBuffer> transport_buf(new net::IOBuffer(len));
-    memcpy(transport_buf->data(), buf, len);
+  }
 
-    int result = ssl_socket_->Write(transport_buf, len,
-                                    net::CompletionCallback());
-    if (result == net::ERR_IO_PENDING) {
-      SetError(EWOULDBLOCK);
-    }
-    transport_buf = NULL;
-    return result;
+  if (write_pending_) {
+    SetError(EWOULDBLOCK);
+    return -1;
   }
+
+  write_buffer_ = new net::DrainableIOBuffer(new net::IOBuffer(len), len);
+  memcpy(write_buffer_->data(), buf, len);
+
+  DoWrite();
+
+  return len;
 }
 
 int SSLSocketAdapter::Recv(void* buf, size_t len) {
   switch (ssl_state_) {
-    case SSLSTATE_NONE:
+    case SSLSTATE_NONE: {
       return AsyncSocketAdapter::Recv(buf, len);
+    }
 
-    case SSLSTATE_WAIT:
+    case SSLSTATE_WAIT: {
       SetError(EWOULDBLOCK);
       return -1;
+    }
 
-    case SSLSTATE_CONNECTED:
-      switch (read_state_) {
-        case IOSTATE_NONE: {
-          transport_buf_ = new net::IOBuffer(len);
-          int result = ssl_socket_->Read(
-              transport_buf_, len,
-              base::Bind(&SSLSocketAdapter::OnRead, base::Unretained(this)));
-          if (result >= 0) {
-            memcpy(buf, transport_buf_->data(), len);
-          }
+    case SSLSTATE_CONNECTED: {
+      if (read_pending_) {
+        SetError(EWOULDBLOCK);
+        return -1;

[Wez, 2012/06/13 00:48:57]

nit: Define a constant for the error return-valie that libjingle expects?

[Sergey Ulanov, 2012/06/13 01:30:31]

Don't think it is necessary. The current code is consistent with libjingle code
and with some parts of net/

+      }
 
-          if (result == net::ERR_IO_PENDING) {
-            read_state_ = IOSTATE_PENDING;
-            SetError(EWOULDBLOCK);
-          } else {
-            if (result < 0) {
-              SetError(result);
-              VLOG(1) << "Socket error " << result;
-            }
-            transport_buf_ = NULL;
-          }
-          return result;
+      int bytes_read = 0;
+
+      if (read_buffer_) {

[Wez, 2012/06/13 00:48:57]

nit:Add a comment to clarify that this is the case where we have some
previously-read data buffered that we can return immediately to the caller?

[Sergey Ulanov, 2012/06/13 01:30:31]

Done.

+        int size = std::min(read_buffer_->RemainingCapacity(),
+                            static_cast<int>(len));
+        memcpy(buf, read_buffer_->data(), size);
+        read_buffer_->set_offset(read_buffer_->offset() + size);
+        if (!read_buffer_->RemainingCapacity())

[Wez, 2012/06/13 00:48:57]

nit: Inconsistent use of {} in ifs.

[Sergey Ulanov, 2012/06/13 01:30:31]

It is consistent - all other single-line ifs in this file don't have {}

+          read_buffer_ = NULL;
+
+        if (size == static_cast<int>(len))

[Wez, 2012/06/13 00:48:57]

nit: Ditto

[Sergey Ulanov, 2012/06/13 01:30:31]

Ditto

+          return size;
+
+        // If we haven't filled in the buffer with the data we had
+        // then try to read from again to fill in the buffer space we
+        // have left.

[Wez, 2012/06/13 00:48:57]

nit: Suggest "If we didn't fill the caller's buffer then dispatch a new Read()
now, in case there's already more data ready to return to them."

[Sergey Ulanov, 2012/06/13 01:30:31]

Done.

+        buf = reinterpret_cast<char*>(buf) + size;
+        len -= size;
+        bytes_read = size;
+        DCHECK(!read_buffer_);
+      }
+
+      read_buffer_ = new net::GrowableIOBuffer();
+      read_buffer_->SetCapacity(len);
+      int result = ssl_socket_->Read(
+          read_buffer_, len,
+          base::Bind(&SSLSocketAdapter::OnRead, base::Unretained(this)));

[Wez, 2012/06/13 00:48:57]

Add a comment before this block to clarify that this is the case where we had
too little data, or no data at all, with which to satisfy the entire read.

[Sergey Ulanov, 2012/06/13 01:30:31]

Done.

+      if (result >= 0)

[Wez, 2012/06/13 00:48:57]

nit: Inconsistent use of {} on if

[Sergey Ulanov, 2012/06/13 01:30:31]

It is consistent - all other single-line ifs in this file don't have {}

+        memcpy(buf, read_buffer_->data(), len);
+
+      if (result == net::ERR_IO_PENDING) {
+        read_pending_ = true;
+        SetError(EWOULDBLOCK);
+        return bytes_read ? bytes_read : -1;
+      } else {

[Wez, 2012/06/13 00:48:57]

I don't think this needs to be in an else, since the IO_PENDING case returns?

[Sergey Ulanov, 2012/06/13 01:30:31]

Done.

+        if (result < 0) {
+          SetError(EINVAL);
+          ssl_state_ = SSLSTATE_ERROR;
+          LOG(ERROR) << "Error reading from SSL socket " << result;
+          return -1;
         }
-        case IOSTATE_PENDING:
-          SetError(EWOULDBLOCK);
-          return -1;
+        read_buffer_ = NULL;
+      }
+      return result + bytes_read;
+    }
 
-        case IOSTATE_COMPLETE:
-          memcpy(buf, transport_buf_->data(), len);
-          transport_buf_ = NULL;
-          read_state_ = IOSTATE_NONE;
-          return data_transferred_;
-      }
+    case SSLSTATE_ERROR: {
+      SetError(EINVAL);
+      return -1;
+    }
   }
 
   NOTREACHED();
   return -1;
 }
 
 void SSLSocketAdapter::OnConnected(int result) {
   if (result == net::OK) {
     ssl_state_ = SSLSTATE_CONNECTED;
     OnConnectEvent(this);
   } else {
     LOG(WARNING) << "OnConnected failed with error " << result;
   }
 }
 
 void SSLSocketAdapter::OnRead(int result) {
-  DCHECK(read_state_ == IOSTATE_PENDING);
-  read_state_ = IOSTATE_COMPLETE;
-  data_transferred_ = result;
+  DCHECK(read_pending_);
+  read_pending_ = false;
+  if (result > 0) {
+    DCHECK_GE(read_buffer_->capacity(), result);
+    read_buffer_->SetCapacity(result);
+  } else {
+    if (result < 0)
+      ssl_state_ = SSLSTATE_ERROR;
+  }
   AsyncSocketAdapter::OnReadEvent(this);
 }
 
-void SSLSocketAdapter::OnWrite(int result) {
-  DCHECK(write_state_ == IOSTATE_PENDING);
-  write_state_ = IOSTATE_COMPLETE;
-  data_transferred_ = result;
+void SSLSocketAdapter::OnWritten(int result) {
+  DCHECK(write_pending_);
+  write_pending_ = false;
+  if (result >= 0) {
+    write_buffer_->DidConsume(result);
+    if (!write_buffer_->BytesRemaining()) {
+      write_buffer_ = NULL;
+    } else {
+      DoWrite();
+    }
+  } else {
+    ssl_state_ = SSLSTATE_ERROR;
+  }
   AsyncSocketAdapter::OnWriteEvent(this);
 }
 
+void SSLSocketAdapter::DoWrite() {
+  DCHECK_GT(write_buffer_->BytesRemaining(), 0);
+  DCHECK(!write_pending_);
+
+  while (write_buffer_ && !write_pending_) {

[Wez, 2012/06/13 00:48:57]

I'd suggest structuring this as a while (true) and returning in the IO_PENDING,
!BytesRemaining() and (I think?) result < 0 cases.  You could also put the
DidConsume case first, with a continue, so that once you're past that point you
know you're going to return.

[Sergey Ulanov, 2012/06/13 01:30:31]

Done.

+    int result = ssl_socket_->Write(
+        write_buffer_, write_buffer_->BytesRemaining(),
+        base::Bind(&SSLSocketAdapter::OnWritten, base::Unretained(this)));
+
+    if (result == net::ERR_IO_PENDING) {
+      write_pending_ = true;
+    } else if (result < 0) {
+      SetError(EINVAL);
+      ssl_state_ = SSLSTATE_ERROR;
+    } else {
+      write_buffer_->DidConsume(result);
+      if (!write_buffer_->BytesRemaining())
+        write_buffer_ = NULL;
+    }
+  }
+}
+
 void SSLSocketAdapter::OnConnectEvent(talk_base::AsyncSocket* socket) {
   if (ssl_state_ != SSLSTATE_WAIT) {
     AsyncSocketAdapter::OnConnectEvent(socket);
   } else {
     ssl_state_ = SSLSTATE_NONE;
     int result = BeginSSL();
     if (0 != result) {
       // TODO(zork): Handle this case gracefully.
       LOG(WARNING) << "BeginSSL() failed with " << result;
     }
@@ skipping 183 matching lines @@
         write_buffer_len_ = buffer_len;
         return;
       }
     }
     was_used_to_convey_data_ = true;
     callback.Run(result);
   }
 }
 
 }  // namespace remoting