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 316 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.
   SSL_set_mode(ssl_, SSL_MODE_ENABLE_PARTIAL_WRITE |
                      SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);

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

From the chat with davidben, I'm not sure we want to keep ENABLE_PARTIAL_WRITE.

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

Probably. But it's unrelated to this CL so I'll leave it as a TODO.

 
   // the SSL object owns the bio now
   bio = nullptr;
 
   // Do the connect
   err = ContinueSSL();
   if (err != 0)
     goto ssl_error;
 
   return err;
@@ skipping 68 matching lines @@
   if (signal)
     AsyncSocketAdapter::OnCloseEvent(this, err);
 }
 
 void
 OpenSSLAdapter::Cleanup() {
   LOG(LS_INFO) << "Cleanup";
 
   state_ = SSL_NONE;
   ssl_read_needs_write_ = false;
   ssl_write_needs_read_ = false;

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

Should this also clear pending_write_ and pending_data_?

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

I'll do it for consistency, but I don't see a way this class could be used after
Close is called since it closes the underlying socket.

   custom_verification_succeeded_ = false;
 
   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) {
+  // If we're in "pending write" mode, we should only be calling SSL_write with
+  // the pending data.
+  RTC_DCHECK(!pending_write_ || pv == pending_data_.data());
+
+  ssl_write_needs_read_ = false;
+  int ret = SSL_write(ssl_, pv, checked_cast<int>(cb));
+  int error = SSL_get_error(ssl_, ret);
+  switch (error) {
+    case SSL_ERROR_SSL:
+      // Walk down the error stack to find the SSL error.
+      uint32_t error_code;

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

This feels like it should be factored into a function.

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

Done.

+      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);
+      Error("SSL_write", ret ? ret : -1, false);
+      break;
+    case SSL_ERROR_NONE:
+      // Success!

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

Maybe this case should come first?

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

Done.

+      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;
+    default:
+      LOG(LS_WARNING) << "Unknown error from SSL_write: " << error;
+      Error("SSL_write", ret ? ret : -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) &&

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

It seems like this should be a low-level routine - it just takes what it gets
and sends it. I would think the handling of pending_data_ would be done at the
next level up - i.e., in OpenSSLAdapter::Send (similar to how pending_write_ is
handled there).

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

Ok. Somehow the fact that the write failed due to "SSL_ERROR_WANT_READ" or
"SSL_ERROR_WANT_WRITE" needs to be communicated though, so I'll make the error
an out param I guess.

+      !pending_write_) {
+    LOG(LS_WARNING)
+        << "SSL_write couldn't write to the underlying socket; buffering data.";
+    pending_write_ = true;
+    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;
+}
+
 //
 // 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;
   }
 
+  if (pending_write_) {
+    int ret = DoSslWrite(pending_data_.data(), pending_data_.size());
+    if (ret == static_cast<int>(pending_data_.size())) {
+      // We completed sending the data previously passed into SSL_write! Now
+      // we're allowed to send more data.
+      pending_write_ = false;
+    } else {
+      // 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 -1;

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

return SOCKET_ERROR

or just:

return ret;

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

Done.

+    }

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

Might be more clear with an early return:

if (pending_write_) {
  int ret = DoSslWrite(pending_data_.data(), pending_data_.size());
  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 -1;
  }
  // We completed sending the data previously passed into SSL_write! Now
  // we're allowed to send more data.
  pending_write_ = false;
}


Or, better yet, put this in a helper method:


It seems like this should be in a helper method:

bool MaybeRetrySslWrite() {
  if (!pending_write_) {
    return true;
  }
  if (DoSslWrite(pending_data_.data(), pending_data_.size()) !=
      static_cast<int>(pending_data_.size())) {
    return false;
  }
  pending_write_ = true;
  return false;  
}


Then, here just call:

if (!MaybeRetrySslWrite()) {
  // 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;
}

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

I don't like "MaybeRetrySslWrite" because it's not obvious what the return value
means. I did the early return though.

+  }
+
   // OpenSSL will return an error if we try to write zero bytes
   if (cb == 0)
     return 0;
 
-  ssl_write_needs_read_ = false;
-
-  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;
-  }
-
-  return SOCKET_ERROR;
+  return DoSslWrite(pv, cb);
 }
 
 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 24 matching lines @@
   // 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;

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

Should we check for SSL_ERROR_SSL here as well?

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

Done.

   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:
@@ skipping 119 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_write_) {
+    if (DoSslWrite(pending_data_.data(), pending_data_.size()) ==
+        static_cast<int>(pending_data_.size())) {
+      pending_write_ = false;

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

Shouldn't we clear the pending_data_?

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

Would have been pointless before, but since I got rid of pending_write_ it's
necessary.

+    }
+  }

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

With the helper method, this could just be:

MaybeRetrySslWrite();

+
   //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