Fixing SSL error that occurs when underlying socket is blocked.

webrtc/base/ssladapter_unittest.cc

 /*
  *  Copyright 2014 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include <memory>
 #include <string>
 
 #include "webrtc/base/gunit.h"
 #include "webrtc/base/ipaddress.h"
 #include "webrtc/base/socketstream.h"
 #include "webrtc/base/ssladapter.h"
+#include "webrtc/base/sslidentity.h"
 #include "webrtc/base/sslstreamadapter.h"
-#include "webrtc/base/sslidentity.h"
 #include "webrtc/base/stream.h"
+#include "webrtc/base/stringencode.h"
 #include "webrtc/base/virtualsocketserver.h"
 
 static const int kTimeout = 5000;
 
 static rtc::AsyncSocket* CreateSocket(const rtc::SSLMode& ssl_mode) {
   rtc::SocketAddress address(rtc::IPAddress(INADDR_ANY), 0);
 
   rtc::AsyncSocket* socket = rtc::Thread::Current()->
       socketserver()->CreateAsyncSocket(
       address.family(), (ssl_mode == rtc::SSL_MODE_DTLS) ?
@@ skipping 171 matching lines @@
 
   void OnServerSocketReadEvent(rtc::AsyncSocket* socket) {
     // Only a single connection is supported.
     ASSERT_TRUE(ssl_stream_adapter_ == nullptr);
 
     DoHandshake(server_socket_->Accept(nullptr));
   }
 
   void OnSSLStreamAdapterEvent(rtc::StreamInterface* stream, int sig, int err) {
     if (sig & rtc::SE_READ) {
-      char buffer[4096] = "";
+      do {
+        char buffer[4096] = "";
+        size_t read;
+        int error;
 
-      size_t read;
-      int error;
+        // Read data received from the client and store it in our internal

[juberti1, 2017/06/02 05:35:44]

This shouldn't be necessary - we should trigger a new SE_READ even after a
partial read. Otherwise, looping here could cause us to become corebound trying
to read a busy stream.

[Taylor Brandstetter, 2017/06/02 06:49:14]

Hmm. So is this an issue with VirtualSocketServer? Should it be queueing a task
to call SignalReadEvent again if a partial read occurs? See updated patchset.

+        // buffer. Keep reading until Read returns something other than
+        // SR_SUCCESS, since multiple SSL records could be received in a single
+        // TCP packet.
+        rtc::StreamResult r =
+            stream->Read(buffer, sizeof(buffer) - 1, &read, &error);
+        if (r != rtc::SR_SUCCESS) {
+          break;
+        }
 
-      // Read data received from the client and store it in our internal
-      // buffer.
-      rtc::StreamResult r = stream->Read(buffer,
-          sizeof(buffer) - 1, &read, &error);
-      if (r == rtc::SR_SUCCESS) {
         buffer[read] = '\0';
-
         LOG(LS_INFO) << "Server received '" << buffer << "'";
-
         data_ += buffer;
-      }
+      } while (true);
     }
   }
 
  private:
   void DoHandshake(rtc::AsyncSocket* socket) {
     rtc::SocketStream* stream = new rtc::SocketStream(socket);
 
     ssl_stream_adapter_.reset(rtc::SSLStreamAdapter::Create(stream));
 
     ssl_stream_adapter_->SetMode(ssl_mode_);
@@ skipping 91 matching lines @@
 
     rv = server_->Send(message);
     ASSERT_EQ(static_cast<int>(message.length()), rv);
 
     // The client should have received the server's message.
     EXPECT_EQ_WAIT(message, client_->GetReceivedData(), kTimeout);
 
     LOG(LS_INFO) << "Transfer complete.";
   }
 
- private:
+ protected:
   const rtc::SSLMode ssl_mode_;
 
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   rtc::AutoSocketServerThread thread_;
   std::unique_ptr<SSLAdapterTestDummyServer> server_;
   std::unique_ptr<SSLAdapterTestDummyClient> client_;
 
   int handshake_wait_;
 };
 
@@ skipping 32 matching lines @@
 TEST_F(SSLAdapterTestTLS_ECDSA, TestTLSConnect) {
   TestHandshake(true);
 }
 
 // Test transfer between client and server, using RSA
 TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransfer) {
   TestHandshake(true);
   TestTransfer("Hello, world!");
 }
 
+TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransferWithBlockedSocket) {

[juberti1, 2017/06/02 05:35:44]

Put this into a TestTransferWithBlockedSocket function in the fixture class,
perhaps?

[Taylor Brandstetter, 2017/06/02 06:49:14]

I don't think that helps, since I only want to run this test with TLS sockets
(and don't care about RSA vs. ECDSA).

[juberti1, 2017/06/02 17:47:22]

Sure, but I was thinking it would be easiest for future maintainers to have all
the test code that does real work arranged similarly.

+  TestHandshake(true);
+
+  // Tell the underlying socket to simulate being blocked.
+  vss_->SetSendingBlocked(true);
+
+  std::string expected;
+  int rv;
+  // Send messages until the SSL socket adapter starts applying backpressure.
+  // Note that this may not occur immediately since there may be some amount of
+  // intermediate buffering (either in our code or in BoringSSL).
+  for (int i = 0; i < 1024; ++i) {
+    std::string message = "Hello, world: " + rtc::ToString(i);
+    rv = client_->Send(message);
+    if (rv != static_cast<int>(message.size())) {
+      // This test assumes either the whole message or none of it is sent.
+      ASSERT_EQ(-1, rv);
+      break;
+    }
+    expected += message;
+  }
+  // Assert that the loop above exited due to Send returning -1.
+  ASSERT_EQ(-1, rv);

[pthatcher1, 2017/06/02 04:58:22]

Should we verify that sending the same thing while still blocked returns -1 and
doesn't cause double buffering or something broken?

[Taylor Brandstetter, 2017/06/02 06:49:14]

Done.

[pthatcher1, 2017/06/02 13:24:24]

I meant doing something like 


EXPECT_EQ(-1, client_->Send("Hello, world: 1023"));

[Taylor Brandstetter, 2017/06/02 17:10:53]

This already tests that the last message (for which -1 was returned) isn't sent.
Do you just want it to test 2 unsent messages instead of 1? If so, done.

+
+  // Unblock the underlying socket. All of the buffered messages should be sent
+  // without any further action.
+  vss_->SetSendingBlocked(false);
+  EXPECT_EQ_WAIT(expected, server_->GetReceivedData(), kTimeout);
+
+  // Send another message. This previously wasn't working
+  std::string final_message = "Fin.";
+  expected += final_message;
+  EXPECT_EQ(static_cast<int>(final_message.size()),
+            client_->Send(final_message));
+  EXPECT_EQ_WAIT(expected, server_->GetReceivedData(), kTimeout);
+}
+
 // Test transfer between client and server, using ECDSA
 TEST_F(SSLAdapterTestTLS_ECDSA, TestTLSTransfer) {
   TestHandshake(true);
   TestTransfer("Hello, world!");
 }
 
 // Basic tests: DTLS
 
 // Test that handshake works, using RSA
 TEST_F(SSLAdapterTestDTLS_RSA, TestDTLSConnect) {
   TestHandshake(true);
 }
 
 // Test that handshake works, using ECDSA
 TEST_F(SSLAdapterTestDTLS_ECDSA, TestDTLSConnect) {
   TestHandshake(true);
 }
 
 // Test transfer between client and server, using RSA
 TEST_F(SSLAdapterTestDTLS_RSA, TestDTLSTransfer) {
   TestHandshake(true);
   TestTransfer("Hello, world!");
 }
 
 // Test transfer between client and server, using ECDSA
 TEST_F(SSLAdapterTestDTLS_ECDSA, TestDTLSTransfer) {
   TestHandshake(true);
   TestTransfer("Hello, world!");
 }