On Sep 24, 2014, at 6:40 AM, sudhe...@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 <sudhe...@yahoo-inc.com>
> Authored: Wed Sep 24 13:32:56 2014 +0000
> Committer: Sudheer Vinukonda <sudhe...@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);
> +}
>
