ChangeLogTag:Wed Sep 5 20:09:09 UTC 2012 Steve Huston <shuston@riverace.com>

3 files changed, 106 insertions, 115 deletions

diff --git a/ChangeLog b/ChangeLog
index 02e7f3ab377..b8c7abfd64a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+Wed Sep  5 20:09:09 UTC 2012  Steve Huston  <shuston@riverace.com>
+
+        * ace/SSL/SSL_SOCK_Stream.inl (recv_i): Added back in logic to wait
+          for data available to receive when called with a timeout. Thanks
+          to Ossama Othman for helping to sort this out. For reference, the
+          issues involved here are well-explained in "SSL and TLS" by
+          Eric Rescorla, section 8.9.
+
+        * ace/SSL/SSL_SOCK_Stream.cpp (recv_n): Removed the hack-y retry on
+          EWOULDBLOCK - the proper handling of timeouts is now in its proper
+          place in recv_i() - see above.
+
 Mon Aug 27 20:38:00 UTC 2012  Steve Huston  <shuston@riverace.com>
 
         * include/makeinclude/platform_linux.GNU: Make CC and CXX setting
diff --git a/ace/SSL/SSL_SOCK_Stream.cpp b/ace/SSL/SSL_SOCK_Stream.cpp
index d5e2a51e3a4..4bccbb833a5 100644
--- a/ace/SSL/SSL_SOCK_Stream.cpp
+++ b/ace/SSL/SSL_SOCK_Stream.cpp
@@ -409,22 +409,9 @@ ACE_SSL_SOCK_Stream::recv_n (void *buf,
                       timeout);
 
       if (n < 0)
-        {
-          if (errno == EWOULDBLOCK)
-            {
-              // If blocked, try again.
-              n = 0;
-              continue;
-            }
-          else
-            {
-              return -1;
-            }
-        }
+        return -1;
       else if (n == 0)
-        {
-          break;
-        }
+        break;
     }
 
   return ACE_Utils::truncate_cast<ssize_t> (bytes_transferred);
@@ -455,22 +442,9 @@ ACE_SSL_SOCK_Stream::recv_n (void *buf, int len, int flags) const
                       flags);
 
       if (n < 0)
-        {
-          if (errno == EWOULDBLOCK)
-            {
-              // If blocked, try again.
-              n = 0;
-              continue;
-            }
-          else
-            {
-              return -1;
-            }
-        }
+        return -1;
       else if (n == 0)
-        {
-          break;
-        }
+        break;
     }
 
   return ACE_Utils::truncate_cast<ssize_t> (bytes_transferred);
@@ -498,24 +472,10 @@ ACE_SSL_SOCK_Stream::send_n (const void *buf, int len, int flags) const
       n = this->send ((const char*) buf + bytes_transferred,
                       len - bytes_transferred,
                       flags);
-
       if (n < 0)
-        {
-          if (errno == EWOULDBLOCK)
-            {
-              // If blocked, try again.
-              n = 0;
-              continue;
-            }
-          else
-            {
-              return -1;
-            }
-        }
+        return -1;
       else if (n == 0)
-        {
-          break;
-        }
+        break;
     }
 
   return ACE_Utils::truncate_cast<ssize_t> (bytes_transferred);
diff --git a/ace/SSL/SSL_SOCK_Stream.inl b/ace/SSL/SSL_SOCK_Stream.inl
index ca4e48ee22f..a720f07ebc6 100644
--- a/ace/SSL/SSL_SOCK_Stream.inl
+++ b/ace/SSL/SSL_SOCK_Stream.inl
@@ -115,89 +115,108 @@ ACE_SSL_SOCK_Stream::recv_i (void *buf,
     ACE::record_and_set_non_blocking_mode (handle,
                                            val);
 
-  // Only block on select() with a timeout if no data in the
-  // internal OpenSSL buffer is pending read completion for
-  // the same reasons stated above, i.e. all data must be read
-  // before blocking on select().
-  if (timeout != 0
-      && !::SSL_pending (this->ssl_))
-    {
-      if (ACE::enter_recv_timedwait (handle,
-                                     timeout,
-                                     val) == -1)
-        return -1;
-    }
-
+  // Only block to wait for I/O ready with a timeout if all on-hand data
+  // has been consumed. If there is remaining data in the SSL buffers
+  // the socket won't "select" even though SSL_read would complete.
+  // See "SSL and TLS" by Rescorla section 8.9 for more info.
+  bool peeking = false;
+  bool retry = false;
   if (flags)
     {
       if (ACE_BIT_ENABLED (flags, MSG_PEEK))
         {
-          bytes_read = ::SSL_peek (this->ssl_,
-                                   static_cast<char *> (buf),
-                                   ACE_Utils::truncate_cast<int> (n));
+          peeking = true;
         }
       else
         {
           ACE_NOTSUP_RETURN (-1);
         }
     }
-  else
-    {
-      bytes_read = ::SSL_read (this->ssl_,
-                               static_cast<char *> (buf),
-                               ACE_Utils::truncate_cast<int> (n));
-    }
 
-  int const status = ::SSL_get_error (this->ssl_, bytes_read);
-  switch (status)
+  do
     {
-    case SSL_ERROR_NONE:
-      if (timeout != 0)
-        ACE::restore_non_blocking_mode (handle, val);
-
-      return bytes_read;
-
-    case SSL_ERROR_WANT_READ:
-    case SSL_ERROR_WANT_WRITE:
-      errno = EWOULDBLOCK;
-
-      return -1;
-
-    case SSL_ERROR_ZERO_RETURN:
-      if (timeout != 0)
-        ACE::restore_non_blocking_mode (handle, val);
-
-      // The peer has notified us that it is shutting down via the SSL
-      // "close_notify" message so we need to shutdown, too.
-      (void) ::SSL_shutdown (this->ssl_);
-
-      return bytes_read;
-
-    case SSL_ERROR_SYSCALL:
-      if (bytes_read == 0)
-        // An EOF occured but the SSL "close_notify" message was not
-        // sent.  This is a protocol error, but we ignore it.
-        return 0;
-
-      // If not an EOF, then fall through to "default" case.
-
-      // On some platforms (e.g. MS Windows) OpenSSL does not store
-      // the last error in errno so explicitly do so.
-      ACE_OS::set_errno_to_last_error ();
-
-      break;
-
-    default:
-      // Reset errno to prevent previous values (e.g. EWOULDBLOCK)
-      // from being associated with a fatal SSL error.
-      errno = 0;
-
-      ACE_SSL_Context::report_error ();
+      retry = false;
+      if (peeking)
+        {
+          bytes_read = ::SSL_peek (this->ssl_,
+                                   static_cast<char *> (buf),
+                                   ACE_Utils::truncate_cast<int> (n));
+        }
+      else
+        {
+          bytes_read = ::SSL_read (this->ssl_,
+                                   static_cast<char *> (buf),
+                                   ACE_Utils::truncate_cast<int> (n));
+        }
 
-      break;
-    }
+      int const status = ::SSL_get_error (this->ssl_, bytes_read);
+      int substat = 0;
+      switch (status)
+        {
+        case SSL_ERROR_NONE:
+          break;
+
+        case SSL_ERROR_WANT_READ:
+        case SSL_ERROR_WANT_WRITE:
+          if (timeout == 0)
+            {
+              errno = EWOULDBLOCK;
+              bytes_read = -1;
+              break;
+            }
+          substat = ACE::handle_ready (handle,
+                                       timeout,
+                                       status == SSL_ERROR_WANT_READ,
+                                       status == SSL_ERROR_WANT_WRITE,
+                                       0);
+          if (substat == 1)   // Now ready to proceed
+            {
+              retry = true;
+              break;
+            }
+          bytes_read = -1;
+          if (substat == 0)
+            errno = ETIME;
+          break;
+
+        case SSL_ERROR_ZERO_RETURN:
+          bytes_read = 0;
+
+          // The peer has notified us that it is shutting down via the SSL
+          // "close_notify" message so we need to shutdown, too.
+          (void) ::SSL_shutdown (this->ssl_);
+
+          break;
+
+        case SSL_ERROR_SYSCALL:
+          if (bytes_read == 0)
+            // An EOF occured but the SSL "close_notify" message was not
+            // sent.  This is a protocol error, but we ignore it.
+            break;
+
+          // If not an EOF, then fall through to "default" case.
+
+          // On some platforms (e.g. MS Windows) OpenSSL does not store
+          // the last error in errno so explicitly do so.
+          ACE_OS::set_errno_to_last_error ();
+
+          break;
+
+        default:
+          // Reset errno to prevent previous values (e.g. EWOULDBLOCK)
+          // from being associated with a fatal SSL error.
+          bytes_read = -1;
+          errno = 0;
+
+          ACE_SSL_Context::report_error ();
+
+          break;
+        }
+    } while (retry);
 
-  return -1;
+  if (timeout != 0)
+    ACE::restore_non_blocking_mode (handle, val);
+  return bytes_read;
 }
 
 ACE_INLINE ssize_t