Fixing SSL error that occurs when underlying socket is blocked.

webrtc/base/openssladapter.cc

Index: webrtc/base/openssladapter.cc
diff --git a/webrtc/base/openssladapter.cc b/webrtc/base/openssladapter.cc
index bc7b99b97a61449ab785eb24c8697e0dcfdf485b..d40e69b8fb4fc39c6c313f521a59df99aba9c426 100644
--- a/webrtc/base/openssladapter.cc
+++ b/webrtc/base/openssladapter.cc
@@ -334,6 +334,11 @@ OpenSSLAdapter::BeginSSL() {
   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.

 
@@ -441,6 +446,80 @@ OpenSSLAdapter::Cleanup() {
   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
 //
@@ -466,38 +545,25 @@ OpenSSLAdapter::Send(const void* pv, size_t cb) {
     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
@@ -682,6 +748,15 @@ OpenSSLAdapter::OnWriteEvent(AsyncSocket* socket) {
     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);
 }