Fix instability in SSL client/server sockets

net/socket/ssl_server_socket_unittest.cc

 // Copyright (c) 2011 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.
 
 // This test suite uses SSLClientSocket to test the implementation of
 // SSLServerSocket. In order to establish connections between the sockets
 // we need two additional classes:
 // 1. FakeSocket
 //    Connects SSL socket to FakeDataChannel. This class is just a stub.
 //
 // 2. FakeDataChannel
 //    Implements the actual exchange of data between two FakeSockets.
 //
 // Implementations of these two classes are included in this file.
 
 #include "net/socket/ssl_server_socket.h"
 
+#include <stdlib.h>
+
 #include <queue>
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "crypto/nss_util.h"
 #include "crypto/rsa_private_key.h"
 #include "net/base/address_list.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verifier.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/ssl_config_service.h"
 #include "net/base/ssl_info.h"
 #include "net/base/x509_certificate.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/socket_test_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/stream_socket.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
 namespace net {
 
 namespace {
 
 class FakeDataChannel {
  public:
-  FakeDataChannel() : read_callback_(NULL), read_buf_len_(0) {
+  FakeDataChannel()
+      : read_callback_(NULL),
+        read_buf_len_(0),
+        ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {

[willchan no longer on Chromium, 2011/07/21 08:16:06]

Need compiler_specific.h for this macro.

[Sergey Ulanov, 2011/07/21 17:11:12]

Done.

   }
 
   virtual int Read(IOBuffer* buf, int buf_len,
                    CompletionCallback* callback) {
     if (data_.empty()) {
       read_callback_ = callback;
       read_buf_ = buf;
       read_buf_len_ = buf_len;
       return net::ERR_IO_PENDING;
     }
     return PropogateData(buf, buf_len);
   }
 
   virtual int Write(IOBuffer* buf, int buf_len,
                     CompletionCallback* callback) {
     data_.push(new net::DrainableIOBuffer(buf, buf_len));
-    DoReadCallback();
+    MessageLoop::current()->PostTask(
+        FROM_HERE, task_factory_.NewRunnableMethod(
+            &FakeDataChannel::DoReadCallback));

[wtc, 2011/07/21 00:27:12]

willchan: can you review the use of ScopedRunnableMethodFactory
in this class?

sergeyu: why do you need to post a task to run DoReadCallback()?

[Sergey Ulanov, 2011/07/21 00:51:15]

Because DoReadCallback() calls callback on the socket on the other end, which
then may then send some data back which would invoke callback on the first
socket. I.e. SSL socket calls Write() on the fake socket, and receives read
callback before Write() returns - this is not expected by SSL sockets code, so
it crashes.

[willchan no longer on Chromium, 2011/07/21 08:16:06]

Looks reasonable to me.

     return buf_len;
   }
 
  private:
   void DoReadCallback() {
-    if (!read_callback_)
+    if (!read_callback_ || !data_.size())

[wtc, 2011/07/21 00:27:12]

Nit: some people prefer data_.empty().

Why do you need to check if data_ is empty?
It seems to imply that you are calling DoReadCallback
more times than you should.

[Sergey Ulanov, 2011/07/21 00:51:15]

Done.

       return;
 
     int copied = PropogateData(read_buf_, read_buf_len_);
     net::CompletionCallback* callback = read_callback_;
     read_callback_ = NULL;
     read_buf_ = NULL;
     read_buf_len_ = 0;
     callback->Run(copied);
   }
 
   int PropogateData(scoped_refptr<net::IOBuffer> read_buf, int read_buf_len) {
     scoped_refptr<net::DrainableIOBuffer> buf = data_.front();
     int copied = std::min(buf->BytesRemaining(), read_buf_len);
     memcpy(read_buf->data(), buf->data(), copied);
     buf->DidConsume(copied);
 
     if (!buf->BytesRemaining())
       data_.pop();
     return copied;
   }
 
   net::CompletionCallback* read_callback_;
   scoped_refptr<net::IOBuffer> read_buf_;
   int read_buf_len_;
 
   std::queue<scoped_refptr<net::DrainableIOBuffer> > data_;
 
+  ScopedRunnableMethodFactory<FakeDataChannel> task_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeDataChannel);
 };
 
 class FakeSocket : public StreamSocket {
  public:
   FakeSocket(FakeDataChannel* incoming_channel,
              FakeDataChannel* outgoing_channel)
       : incoming_(incoming_channel),
         outgoing_(outgoing_channel) {
   }
 
   virtual ~FakeSocket() {
 
   }
 
   virtual int Read(IOBuffer* buf, int buf_len,
                    CompletionCallback* callback) {
+    // Read random number of bytes.
+    buf_len = rand() % buf_len + 1;
     return incoming_->Read(buf, buf_len, callback);
   }
 
   virtual int Write(IOBuffer* buf, int buf_len,
                     CompletionCallback* callback) {
+    // Write random number of bytes.
+    buf_len = rand() % buf_len + 1;

[wtc, 2011/07/21 00:27:12]

Are you using rand() instead of base::RandInt() because
you need a repeatable sequence of pseudorandom numbers?

     return outgoing_->Write(buf, buf_len, callback);
   }
 
   virtual bool SetReceiveBufferSize(int32 size) {
     return true;
   }
 
   virtual bool SetSendBufferSize(int32 size) {
     return true;
   }
@@ skipping 65 matching lines @@
   FakeSocket client(&channel_1, &channel_2);
   FakeSocket server(&channel_2, &channel_1);
 
   const char kTestData[] = "testing123";
   const int kTestDataSize = strlen(kTestData);
   const int kReadBufSize = 1024;
   scoped_refptr<net::IOBuffer> write_buf = new net::StringIOBuffer(kTestData);
   scoped_refptr<net::IOBuffer> read_buf = new net::IOBuffer(kReadBufSize);
 
   // Write then read.
-  EXPECT_EQ(kTestDataSize, server.Write(write_buf, kTestDataSize, NULL));
-  EXPECT_EQ(kTestDataSize, client.Read(read_buf, kReadBufSize, NULL));
-  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), kTestDataSize));
+  int written = server.Write(write_buf, kTestDataSize, NULL);
+  EXPECT_GT(written, 0);
+  EXPECT_LE(written, kTestDataSize);
+
+  int read = client.Read(read_buf, kReadBufSize, NULL);

[wtc, 2011/07/21 00:27:12]

Nit: name these local variables nwritten and nread
or bytes_written and bytes_read.  "read" is the name of
a Unix system call.

+  EXPECT_GT(read, 0);
+  EXPECT_LE(read, written);
+  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 
   // Read then write.
   TestCompletionCallback callback;
   EXPECT_EQ(net::ERR_IO_PENDING,
             server.Read(read_buf, kReadBufSize, &callback));
-  EXPECT_EQ(kTestDataSize, client.Write(write_buf, kTestDataSize, NULL));
-  EXPECT_EQ(kTestDataSize, callback.WaitForResult());
-  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), kTestDataSize));
+
+  written = client.Write(write_buf, kTestDataSize, NULL);
+  EXPECT_GT(written, 0);
+  EXPECT_LE(written, kTestDataSize);
+
+  read = callback.WaitForResult();
+  EXPECT_GT(read, 0);
+  EXPECT_LE(read, written);
+  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 }
 
 class SSLServerSocketTest : public PlatformTest {
  public:
   SSLServerSocketTest()
       : socket_factory_(net::ClientSocketFactory::GetDefaultFactory()) {
   }
 
  protected:
   void Initialize() {
@@ skipping 147 matching lines @@
     EXPECT_GT(read_callback.WaitForResult(), 0);
   }
   if (client_ret == net::ERR_IO_PENDING) {
     EXPECT_GT(write_callback.WaitForResult(), 0);
   }
   EXPECT_EQ(0, memcmp(write_buf->data(), read_buf->data(), write_buf->size()));
 }
 #endif
 
 }  // namespace net