trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: git commit: [TS-3085] Large POSTs over (relatively) slower connections failing in ats5 Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up the error queue after processing an SSL_ERROR_SSL. Also, added consistent wrappers to SSL I/O
Date Wed, 24 Sep 2014 15:54:36 GMT
On Sep 24, 2014, at 6:40 AM, sudheerv@apache.org wrote:

> Repository: trafficserver
> Updated Branches:
>  refs/heads/master 17bef772a -> d12327d84
> 
> 
> [TS-3085] Large POSTs over (relatively) slower connections failing in ats5
> Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up
> the error queue after processing an SSL_ERROR_SSL. Also, added consistent
> wrappers to SSL I/O functions and removed errno based looping on SSL_Write
> per James Peach's recommendations.  Note that, this change removes the apparent
> logic of reattempting SSL_Write on transient error cases (such as ENOBUF), since
> openSSL documentation doesn't indicate that errno is set during SSL_Write.
> https://www.openssl.org/docs/ssl/SSL_write.html
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d12327d8
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d12327d8
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d12327d8
> 
> Branch: refs/heads/master
> Commit: d12327d841d07bfe1fa772c42272fbc908a39324
> Parents: 17bef77
> Author: Sudheer Vinukonda <sudheerv@yahoo-inc.com>
> Authored: Wed Sep 24 13:32:56 2014 +0000
> Committer: Sudheer Vinukonda <sudheerv@yahoo-inc.com>
> Committed: Wed Sep 24 13:32:56 2014 +0000
> 
> ----------------------------------------------------------------------
> iocore/net/P_SSLUtils.h         | 12 +++++++
> iocore/net/SSLNetVConnection.cc | 69 ++++++++++++------------------------
> iocore/net/SSLUtils.cc          | 59 ++++++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+), 47 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/P_SSLUtils.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h
> index 82c41b4..269f009 100644
> --- a/iocore/net/P_SSLUtils.h
> +++ b/iocore/net/P_SSLUtils.h
> @@ -38,6 +38,8 @@ struct SSLCertLookup;
> class SSLNetVConnection;
> struct RecRawStatBlock;
> 
> +typedef int ssl_error_t;
> +
> enum SSL_Stats
> {
>   ssl_origin_server_expired_cert_stat,
> @@ -113,6 +115,12 @@ void SSLInitializeStatistics();
> // Release SSL_CTX and the associated data
> void SSLReleaseContext(SSL_CTX* ctx);
> 
> +// Wrapper functions to SSL I/O routines
> +ssl_error_t SSLWriteBuffer(SSL * ssl, const void * buf, size_t nbytes, size_t& nwritten);
> +ssl_error_t SSLReadBuffer(SSL * ssl, void * buf, size_t nbytes, size_t& nread);
> +ssl_error_t SSLAccept(SSL *ssl);
> +ssl_error_t SSLConnect(SSL * ssl);
> +
> // Log an SSL error.
> #define SSLError(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, NULL, fmt, ##__VA_ARGS__)
> #define SSLErrorVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, vc, fmt, ##__VA_ARGS__)
> @@ -120,6 +128,10 @@ void SSLReleaseContext(SSL_CTX* ctx);
> #define SSLDebug(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, NULL, fmt, ##__VA_ARGS__)
> #define SSLDebugVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, vc, fmt, ##__VA_ARGS__)
> 
> +#define SSL_CLR_ERR_INCR_DYN_STAT(x, fmt, ...) \
> +  SSLDiagnostic(DiagsMakeLocation(), true, NULL, fmt, ##__VA_ARGS__); \
> +  RecIncrRawStat(ssl_rsb, NULL, (int) x, 1);
> +

Pleas wrap this in a do { } while(0) ...

> void SSLDiagnostic(const SrcLoc& loc, bool debug, SSLNetVConnection * vc, const char
* fmt, ...) TS_PRINTFLIKE(4, 5);
> 
> // Return a static string name for a SSL_ERROR constant.
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/SSLNetVConnection.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
> index b4269e7..0f26679 100644
> --- a/iocore/net/SSLNetVConnection.cc
> +++ b/iocore/net/SSLNetVConnection.cc
> @@ -172,24 +172,6 @@ debug_certificate_name(const char * msg, X509_NAME * name)
>   BIO_free(bio);
> }
> 
> -static inline int
> -do_SSL_write(SSL * ssl, void *buf, int size)
> -{
> -  int r = 0;
> -  do {
> -    // need to check into SSL error handling
> -    // to see if this is good enough.
> -    r = SSL_write(ssl, (const char *) buf, size);
> -    if (r >= 0)
> -      return r;
> -    else
> -      r = -errno;
> -  } while (r == -EINTR || r == -ENOBUFS || r == -ENOMEM);
> -
> -  return r;
> -}
> -
> -
> static int
> ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t &ret)
> {
> @@ -199,7 +181,8 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t
&ret)
>   int event = SSL_READ_ERROR_NONE;
>   int64_t bytes_read;
>   int64_t block_write_avail;
> -  int sslErr = SSL_ERROR_NONE;
> +  ssl_error_t sslErr = SSL_ERROR_NONE;
> +  int nread = 0;
> 
>   for (bytes_read = 0; (b != 0) && (sslErr == SSL_ERROR_NONE); b = b->next)
{
>     block_write_avail = b->write_avail();
> @@ -209,23 +192,22 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread,
int64_t &ret)
>     int64_t offset = 0;
>     // while can be replaced with if - need to test what works faster with openssl
>     while (block_write_avail > 0) {
> -      int rres = SSL_read(sslvc->ssl, b->end() + offset, (int)block_write_avail);
> +      sslErr = SSLReadBuffer (sslvc->ssl, b->end() + offset, (size_t)block_write_avail,
(size_t&)nread);

Don't cast this, just use size_t.

> 
> -      Debug("ssl", "[SSL_NetVConnection::ssl_read_from_net] rres=%d", rres);
> +      Debug("ssl", "[SSL_NetVConnection::ssl_read_from_net] nread=%d", (int)nread);
> 
> -      sslErr = SSL_get_error(sslvc->ssl, rres);
>       switch (sslErr) {
>       case SSL_ERROR_NONE:
> 
> #if DEBUG
> -        SSLDebugBufferPrint("ssl_buff", b->end() + offset, rres, "SSL Read");
> +        SSLDebugBufferPrint("ssl_buff", b->end() + offset, nread, "SSL Read");
> #endif
> 
> -        ink_assert(rres);
> +        ink_assert(nread);
> 
> -        bytes_read += rres;
> -        offset += rres;
> -        block_write_avail -= rres;
> +        bytes_read += nread;
> +        offset += nread;
> +        block_write_avail -= nread;
>         ink_assert(block_write_avail >= 0);
> 
>         continue;
> @@ -247,7 +229,7 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t
&ret)
>         break;
>       case SSL_ERROR_SYSCALL:
>         SSL_INCREMENT_DYN_STAT(ssl_error_syscall);
> -        if (rres != 0) {
> +        if (nread != 0) {
>           // not EOF
>           event = SSL_READ_ERROR;
>           ret = errno;
> @@ -267,8 +249,7 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t
&ret)
>       default:
>         event = SSL_READ_ERROR;
>         ret = errno;
> -        SSL_INCREMENT_DYN_STAT(ssl_error_ssl);
> -        Debug("ssl.error", "[SSL_NetVConnection::ssl_read_from_net]");
> +        SSL_CLR_ERR_INCR_DYN_STAT(ssl_error_ssl, "[SSL_NetVConnection::ssl_read_from_net]:
errno=%d", errno);
>         break;
>       }                         // switch
>       break;
> @@ -636,6 +617,7 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, int64_t
&wattempted, i
>   ProxyMutex *mutex = this_ethread()->mutex;
>   int64_t r = 0;
>   int64_t l = 0;
> +  ssl_error_t err = SSL_ERROR_NONE;
> 
>   // XXX Rather than dealing with the block directly, we should use the IOBufferReader
API.
>   int64_t offset = buf.reader()->start_offset;
> @@ -675,9 +657,9 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, int64_t
&wattempted, i
> 
>     wattempted = l;
>     total_wrote += l;
> -    Debug("ssl", "SSLNetVConnection::loadBufferAndCallWrite, before do_SSL_write, l=%"
PRId64", towrite=%" PRId64", b=%p",
> +    Debug("ssl", "SSLNetVConnection::loadBufferAndCallWrite, before SSLWriteBuffer,
l=%" PRId64", towrite=%" PRId64", b=%p",
>           l, towrite, b);
> -    r = do_SSL_write(ssl, b->start() + offset, (int)l);
> +    err = SSLWriteBuffer(ssl, b->start() + offset, (size_t)l, (size_t&)r);

Again, this cast is unsafe.

>     if (r == l) {
>       wattempted = total_wrote;
>     }
> @@ -704,8 +686,6 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, int64_t
&wattempted, i
>       return (total_wrote);
>     }
>   } else {
> -    int err = SSL_get_error(ssl, (int)r);
> -
>     switch (err) {
>     case SSL_ERROR_NONE:
>       Debug("ssl", "SSL_write-SSL_ERROR_NONE");
> @@ -742,8 +722,7 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, int64_t
&wattempted, i
>     case SSL_ERROR_SSL:
>     default:
>       r = -errno;
> -      SSL_INCREMENT_DYN_STAT(ssl_error_ssl);
> -      Debug("ssl.error", "SSL_write-SSL_ERROR_SSL");
> +      SSL_CLR_ERR_INCR_DYN_STAT(ssl_error_ssl, "SSL_write-SSL_ERROR_SSL errno=%d", errno);
>       break;
>     }
>     return (r);
> @@ -880,8 +859,6 @@ SSLNetVConnection::sslStartHandShake(int event, int &err)
> int
> SSLNetVConnection::sslServerHandShakeEvent(int &err)
> {
> -  int ret;
> -
>   if (SSL_HOOKS_DONE != sslPreAcceptHookState) {
>     // Get the first hook if we haven't started invoking yet.
>     if (SSL_HOOKS_INIT == sslPreAcceptHookState) {
> @@ -939,8 +916,7 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
>     }
>   }
> 
> -  ret = SSL_accept(ssl);
> -  int ssl_error = SSL_get_error(ssl, ret);
> +  ssl_error_t ssl_error = SSLAccept(ssl);
> 
>   if (ssl_error != SSL_ERROR_NONE) {
>     err = errno;
> @@ -1039,9 +1015,11 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
>   case SSL_ERROR_WANT_X509_LOOKUP:
>     return EVENT_CONT;
> 
> +  case SSL_ERROR_SSL:
> +    SSL_CLR_ERR_INCR_DYN_STAT(ssl_error_ssl, "SSLNetVConnection::sslServerHandShakeEvent,
SSL_ERROR_SSL errno=%d", errno);
> +    // fall through
>   case SSL_ERROR_ZERO_RETURN:
>   case SSL_ERROR_SYSCALL:
> -  case SSL_ERROR_SSL:
>   default:
>     return EVENT_ERROR;
>   }
> @@ -1052,8 +1030,6 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
> int
> SSLNetVConnection::sslClientHandShakeEvent(int &err)
> {
> -  int ret;
> -
> #if TS_USE_TLS_SNI
>   if (options.sni_servername) {
>     if (SSL_set_tlsext_host_name(ssl, options.sni_servername)) {
> @@ -1065,8 +1041,8 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
>   }
> #endif
> 
> -  ret = SSL_connect(ssl);
> -  switch (SSL_get_error(ssl, ret)) {
> +  ssl_error_t ssl_error = SSLConnect(ssl);
> +  switch (ssl_error) {
>   case SSL_ERROR_NONE:
>     if (is_debug_tag_set("ssl")) {
>       X509 * cert = SSL_get_peer_certificate(ssl);
> @@ -1122,8 +1098,7 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
>   case SSL_ERROR_SSL:
>   default:
>     err = errno;
> -    SSL_INCREMENT_DYN_STAT(ssl_error_ssl);
> -    Debug("ssl.error", "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_SSL");
> +    SSL_CLR_ERR_INCR_DYN_STAT(ssl_error_ssl, "SSLNetVConnection::sslClientHandShakeEvent,
SSL_ERROR_SSL errno=%d", errno);
>     return EVENT_ERROR;
>     break;
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/SSLUtils.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
> index a57ce90..59e0b63 100644
> --- a/iocore/net/SSLUtils.cc
> +++ b/iocore/net/SSLUtils.cc
> @@ -1660,3 +1660,62 @@ SSLReleaseContext(SSL_CTX * ctx)
> {
>   SSL_CTX_free(ctx);
> }
> +
> +
> +ssl_error_t
> +SSLWriteBuffer(SSL * ssl, const void * buf, size_t nbytes, size_t& nwritten)
> +{
> +  nwritten = 0;
> +
> +  if (unlikely(nbytes == 0)) {
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  int ret = SSL_write(ssl, buf, (int)nbytes);
> +  if (ret > 0) {
> +    nwritten = ret;
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  return SSL_get_error(ssl, ret);
> +}
> +
> +ssl_error_t
> +SSLReadBuffer(SSL * ssl, void * buf, size_t nbytes, size_t& nread)
> +{
> +  nread = 0;
> +
> +  if (unlikely(nbytes == 0)) {
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  int ret = SSL_read(ssl, buf, (int)nbytes);
> +  if (ret > 0) {
> +    nread = ret;
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  return SSL_get_error(ssl, ret);
> +}
> +
> +ssl_error_t
> +SSLAccept(SSL * ssl)
> +{
> +  int ret = SSL_accept(ssl);
> +  if (ret > 0) {
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  return SSL_get_error(ssl, ret);
> +}
> +
> +ssl_error_t
> +SSLConnect(SSL * ssl)
> +{
> +  int ret = SSL_connect(ssl);
> +  if (ret > 0) {
> +    return SSL_ERROR_NONE;
> +  }
> +
> +  return SSL_get_error(ssl, ret);
> +}
> 


Mime
View raw message