Fixing SSL error that occurs when underlying socket is blocked.

webrtc/base/openssladapter.cc

 /*
  *  Copyright 2008 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.
  */
 
@@ skipping 136 matching lines @@
   case BIO_CTRL_WPENDING:
   case BIO_CTRL_PENDING:
     return 0;
   case BIO_CTRL_FLUSH:
     return 1;
   default:
     return 0;
   }
 }
 
+static void LogSslError() {
+  // Walk down the error stack to find the SSL error.
+  uint32_t error_code;
+  const char* file;
+  int line;
+  do {
+    error_code = ERR_get_error_line(&file, &line);
+    if (ERR_GET_LIB(error_code) == ERR_LIB_SSL) {
+      LOG(LS_ERROR) << "ERR_LIB_SSL: " << error_code << ", " << file << ":"
+                    << line;
+      break;
+    }
+  } while (error_code != 0);
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // OpenSSLAdapter
 /////////////////////////////////////////////////////////////////////////////
 
 namespace rtc {
 
 #ifndef OPENSSL_IS_BORINGSSL
 
 // This array will store all of the mutexes available to OpenSSL.
 static MUTEX_TYPE* mutex_buf = nullptr;
@@ skipping 160 matching lines @@
 
   ssl_ = SSL_new(ssl_ctx_);
   if (!ssl_) {
     err = -1;
     goto ssl_error;
   }
 
   SSL_set_app_data(ssl_, this);
 
   SSL_set_bio(ssl_, bio, bio);
+  // SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER allows different buffers to be passed
+  // into SSL_write when a record could only be partially transmitted (and thus
+  // requires another call to SSL_write to finish transmission). This allows us
+  // to copy the data into our own buffer when this occurs, since the original
+  // buffer can't safely be accessed after control exits Send.
+  // TODO(deadbeef): Do we want SSL_MODE_ENABLE_PARTIAL_WRITE? It doesn't
+  // appear Send handles partial writes properly, though maybe we never notice
+  // since we never send more than 16KB at once..
   SSL_set_mode(ssl_, SSL_MODE_ENABLE_PARTIAL_WRITE |
                      SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
 
   // the SSL object owns the bio now
   bio = nullptr;
 
   // Do the connect
   err = ContinueSSL();
   if (err != 0)
     goto ssl_error;
@@ skipping 72 matching lines @@
 }
 
 void
 OpenSSLAdapter::Cleanup() {
   LOG(LS_INFO) << "Cleanup";
 
   state_ = SSL_NONE;
   ssl_read_needs_write_ = false;
   ssl_write_needs_read_ = false;
   custom_verification_succeeded_ = false;
+  pending_data_.Clear();
 
   if (ssl_) {
     SSL_free(ssl_);
     ssl_ = nullptr;
   }
 
   if (ssl_ctx_) {
     SSL_CTX_free(ssl_ctx_);
     ssl_ctx_ = nullptr;
   }
 
   // Clear the DTLS timer
   Thread::Current()->Clear(this, MSG_TIMEOUT);
 }
 
+int OpenSSLAdapter::DoSslWrite(const void* pv, size_t cb, int* error) {
+  // If we have pending data (that was previously only partially written by
+  // SSL_write), we shouldn't be attempting to write anything else.
+  RTC_DCHECK(pending_data_.empty() || pv == pending_data_.data());
+  RTC_DCHECK(error != nullptr);
+
+  ssl_write_needs_read_ = false;
+  int ret = SSL_write(ssl_, pv, checked_cast<int>(cb));
+  *error = SSL_get_error(ssl_, ret);
+  switch (*error) {
+    case SSL_ERROR_NONE:
+      // Success!
+      return ret;
+    case SSL_ERROR_WANT_READ:
+      LOG(LS_INFO) << " -- error want read";
+      ssl_write_needs_read_ = true;
+      SetError(EWOULDBLOCK);
+      break;
+    case SSL_ERROR_WANT_WRITE:
+      LOG(LS_INFO) << " -- error want write";
+      SetError(EWOULDBLOCK);
+      break;
+    case SSL_ERROR_ZERO_RETURN:
+      // LOG(LS_INFO) << " -- remote side closed";
+      SetError(EWOULDBLOCK);
+      // do we need to signal closure?
+      break;
+    case SSL_ERROR_SSL:
+      LogSslError();
+      Error("SSL_write", ret ? ret : -1, false);
+      break;
+    default:
+      LOG(LS_WARNING) << "Unknown error from SSL_write: " << *error;
+      Error("SSL_write", ret ? ret : -1, false);
+      break;
+  }
+
+  return SOCKET_ERROR;
+}
+
 //
 // AsyncSocket Implementation
 //
 
 int
 OpenSSLAdapter::Send(const void* pv, size_t cb) {
   //LOG(LS_INFO) << "OpenSSLAdapter::Send(" << cb << ")";
 
   switch (state_) {
   case SSL_NONE:
     return AsyncSocketAdapter::Send(pv, cb);
 
   case SSL_WAIT:
   case SSL_CONNECTING:
     SetError(ENOTCONN);
     return SOCKET_ERROR;
 
   case SSL_CONNECTED:
     break;
 
   case SSL_ERROR:
   default:
     return SOCKET_ERROR;
   }
 
+  int ret;
+  int error;
+
+  if (!pending_data_.empty()) {
+    ret = DoSslWrite(pending_data_.data(), pending_data_.size(), &error);
+    if (ret != static_cast<int>(pending_data_.size())) {
+      // We couldn't finish sending the pending data, so we definitely can't
+      // send any more data. Return with an EWOULDBLOCK error.
+      SetError(EWOULDBLOCK);
+      return SOCKET_ERROR;
+    }
+    // We completed sending the data previously passed into SSL_write! Now
+    // we're allowed to send more data.
+    pending_data_.Clear();
+  }
+
   // OpenSSL will return an error if we try to write zero bytes
   if (cb == 0)
     return 0;
 
-  ssl_write_needs_read_ = false;
+  ret = DoSslWrite(pv, cb, &error);
 
-  int code = SSL_write(ssl_, pv, checked_cast<int>(cb));
-  switch (SSL_get_error(ssl_, code)) {
-  case SSL_ERROR_NONE:
-    //LOG(LS_INFO) << " -- success";
-    return code;
-  case SSL_ERROR_WANT_READ:
-    //LOG(LS_INFO) << " -- error want read";
-    ssl_write_needs_read_ = true;
-    SetError(EWOULDBLOCK);
-    break;
-  case SSL_ERROR_WANT_WRITE:
-    //LOG(LS_INFO) << " -- error want write";
-    SetError(EWOULDBLOCK);
-    break;
-  case SSL_ERROR_ZERO_RETURN:
-    //LOG(LS_INFO) << " -- remote side closed";
-    SetError(EWOULDBLOCK);
-    // do we need to signal closure?
-    break;
-  default:
-    //LOG(LS_INFO) << " -- error " << code;
-    Error("SSL_write", (code ? code : -1), false);
-    break;
+  // If SSL_write fails with SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE, this
+  // means the underlying socket is blocked on reading or (more typically)
+  // writing. When this happens, OpenSSL requires that the next call to
+  // SSL_write uses the same arguments (though, with
+  // SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER, the actual buffer pointer may be
+  // different).
+  //
+  // However, after Send exits, we will have lost access to data the user of
+  // this class is trying to send, and there's no guarantee that the user of
+  // this class will call Send with the same arguements when it fails. So, we
+  // buffer the data ourselves. When we know the underlying socket is writable
+  // again from OnWriteEvent (or if Send is called again before that happens),
+  // we'll retry sending this buffered data.
+  if ((error == SSL_ERROR_WANT_READ || error == SSL_ERROR_WANT_WRITE) &&
+      pending_data_.empty()) {

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

Is it possible for pending_data_ to *not* be empty here?

[Taylor Brandstetter, 2017/06/02 18:18:42]

Oh, it isn't possible now that this code was shuffled around. I'll make a
follow-up CL that adds a DCHECK.

+    LOG(LS_WARNING)
+        << "SSL_write couldn't write to the underlying socket; buffering data.";
+    pending_data_.SetData(static_cast<const uint8_t*>(pv), cb);
+    // Since we're taking responsibility for sending this data, return its full
+    // size. The user of this class can consider it sent.
+    return cb;
   }
 
-  return SOCKET_ERROR;
+  return ret;
 }
 
 int
 OpenSSLAdapter::SendTo(const void* pv, size_t cb, const SocketAddress& addr) {
   if (socket_->GetState() == Socket::CS_CONNECTED &&
       addr == socket_->GetRemoteAddress()) {
     return Send(pv, cb);
   }
 
   SetError(ENOTCONN);
@@ skipping 21 matching lines @@
     return SOCKET_ERROR;
   }
 
   // Don't trust OpenSSL with zero byte reads
   if (cb == 0)
     return 0;
 
   ssl_read_needs_write_ = false;
 
   int code = SSL_read(ssl_, pv, checked_cast<int>(cb));
-  switch (SSL_get_error(ssl_, code)) {
-  case SSL_ERROR_NONE:
-    //LOG(LS_INFO) << " -- success";
-    return code;
-  case SSL_ERROR_WANT_READ:
-    //LOG(LS_INFO) << " -- error want read";
-    SetError(EWOULDBLOCK);
-    break;
-  case SSL_ERROR_WANT_WRITE:
-    //LOG(LS_INFO) << " -- error want write";
-    ssl_read_needs_write_ = true;
-    SetError(EWOULDBLOCK);
-    break;
-  case SSL_ERROR_ZERO_RETURN:
-    //LOG(LS_INFO) << " -- remote side closed";
-    SetError(EWOULDBLOCK);
-    // do we need to signal closure?
-    break;
-  default:
-    //LOG(LS_INFO) << " -- error " << code;
-    Error("SSL_read", (code ? code : -1), false);
-    break;
+  int error = SSL_get_error(ssl_, code);
+  switch (error) {
+    case SSL_ERROR_NONE:
+      // LOG(LS_INFO) << " -- success";
+      return code;
+    case SSL_ERROR_WANT_READ:
+      // LOG(LS_INFO) << " -- error want read";
+      SetError(EWOULDBLOCK);
+      break;
+    case SSL_ERROR_WANT_WRITE:
+      // LOG(LS_INFO) << " -- error want write";
+      ssl_read_needs_write_ = true;
+      SetError(EWOULDBLOCK);
+      break;
+    case SSL_ERROR_ZERO_RETURN:
+      // LOG(LS_INFO) << " -- remote side closed";
+      SetError(EWOULDBLOCK);
+      // do we need to signal closure?
+      break;
+    case SSL_ERROR_SSL:
+      LogSslError();
+      Error("SSL_read", (code ? code : -1), false);
+      break;
+    default:
+      LOG(LS_WARNING) << "Unknown error from SSL_read: " << error;
+      Error("SSL_read", (code ? code : -1), false);
+      break;
   }
 
   return SOCKET_ERROR;
 }
 
 int OpenSSLAdapter::RecvFrom(void* pv,
                              size_t cb,
                              SocketAddress* paddr,
                              int64_t* timestamp) {
   if (socket_->GetState() == Socket::CS_CONNECTED) {
@@ skipping 101 matching lines @@
     return;
 
   // Don't let ourselves go away during the callbacks
   //PRefPtr<OpenSSLAdapter> lock(this); // TODO: fix this
 
   if (ssl_read_needs_write_)  {
     //LOG(LS_INFO) << " -- onStreamReadable";
     AsyncSocketAdapter::OnReadEvent(socket);
   }
 
+  // If a previous SSL_write failed due to the underlying socket being blocked,
+  // this will attempt finishing the write operation.
+  if (!pending_data_.empty()) {
+    int error;
+    if (DoSslWrite(pending_data_.data(), pending_data_.size(), &error) ==
+        static_cast<int>(pending_data_.size())) {
+      pending_data_.Clear();
+    }
+  }
+
   //LOG(LS_INFO) << " -- onStreamWriteable";
   AsyncSocketAdapter::OnWriteEvent(socket);
 }
 
 void
 OpenSSLAdapter::OnCloseEvent(AsyncSocket* socket, int err) {
   LOG(LS_INFO) << "OpenSSLAdapter::OnCloseEvent(" << err << ")";
   AsyncSocketAdapter::OnCloseEvent(socket, err);
 }
 
@@ skipping 211 matching lines @@
   SSL_CTX_set_cipher_list(ctx, "ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH");
 
   if (ssl_mode_ == SSL_MODE_DTLS) {
     SSL_CTX_set_read_ahead(ctx, 1);
   }
 
   return ctx;
 }
 
 } // namespace rtc