From 960c5c7b27c63021abe3d9ef94aee271f99f8bee Mon Sep 17 00:00:00 2001 From: Trond Norbye Date: Thu, 10 Oct 2019 11:19:30 +0200 Subject: [PATCH] MB-36418: Revert "MB-36028: Flush the SSL socket more aggressively" This caused a throughput decrease in YCSB Workload E This reverts commit 79a67213e7278b15cc3429d7eb2546a4d2b8607e. Change-Id: I067345b56518e92cc76dd0bb318bb2c31d334912 Reviewed-on: http://review.couchbase.org/116206 Tested-by: Build Bot Reviewed-by: Dave Rigby --- daemon/connection.cc | 150 +++++++++++++++++++++++----------- daemon/ssl_context.h | 59 ++----------- daemon/ssl_context_openssl.cc | 124 +++------------------------- 3 files changed, 116 insertions(+), 217 deletions(-) diff --git a/daemon/connection.cc b/daemon/connection.cc index f90b64302a..1557060275 100644 --- a/daemon/connection.cc +++ b/daemon/connection.cc @@ -789,11 +789,7 @@ void Connection::logSslErrorInfo(const std::string& method, int rval) { } int Connection::sslPreConnection() { - int r = ssl.accept(socketDescriptor); - if (ssl.hasError()) { - cb::net::set_econnreset(); - return -1; - } + int r = ssl.accept(); if (r == 1) { ssl.drainBioSendPipe(socketDescriptor); ssl.setConnected(); @@ -845,17 +841,19 @@ int Connection::sslPreConnection() { LOG_INFO("{}: Using SSL cipher:{}", getId(), ssl.getCurrentCipherName()); - return 0; - } - - auto err = ssl.getError(r); - if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE) { - cb::net::set_ewouldblock(); } else { - logSslErrorInfo("SSL_accept", r); - cb::net::set_econnreset(); + if (ssl.getError(r) == SSL_ERROR_WANT_READ) { + ssl.drainBioSendPipe(socketDescriptor); + cb::net::set_ewouldblock(); + return -1; + } else { + logSslErrorInfo("SSL_accept", r); + cb::net::set_econnreset(); + return -1; + } } - return -1; + + return 0; } int Connection::recv(char* dest, size_t nbytes) { @@ -1132,56 +1130,110 @@ Connection::TryReadResult Connection::tryReadNetwork() { } int Connection::sslRead(char* dest, size_t nbytes) { - const auto ret = ssl.read(socketDescriptor, dest, gsl::narrow(nbytes)); - if (ssl.hasError()) { - cb::net::set_econnreset(); - return -1; - } - - if (ret > 0) { - return ret; - } - const int error = ssl.getError(ret); + int ret = 0; - switch (error) { - case SSL_ERROR_WANT_READ: - case SSL_ERROR_WANT_WRITE: - cb::net::set_ewouldblock(); - return -1; + while (ret < int(nbytes)) { + int n; + ssl.drainBioRecvPipe(socketDescriptor); + if (ssl.hasError()) { + cb::net::set_econnreset(); + return -1; + } + n = ssl.read(dest + ret, (int)(nbytes - ret)); + if (n > 0) { + ret += n; + } else { + /* n < 0 and n == 0 require a check of SSL error*/ + const int error = ssl.getError(n); + + switch (error) { + case SSL_ERROR_WANT_READ: + /* + * Drain the buffers and retry if we've got data in + * our input buffers + */ + if (ssl.moreInputAvailable()) { + /* our recv buf has data feed the BIO */ + ssl.drainBioRecvPipe(socketDescriptor); + } else if (ret > 0) { + /* nothing in our recv buf, return what we have */ + return ret; + } else { + cb::net::set_ewouldblock(); + return -1; + } + break; - case SSL_ERROR_ZERO_RETURN: - /* The TLS/SSL connection has been closed (cleanly). */ - return 0; + case SSL_ERROR_ZERO_RETURN: + /* The TLS/SSL connection has been closed (cleanly). */ + return 0; - default: - logSslErrorInfo("SSL_read", ret); - cb::net::set_econnreset(); - return -1; + default: + logSslErrorInfo("SSL_read", n); + cb::net::set_econnreset(); + return -1; + } + } } - // not reached + + return ret; } int Connection::sslWrite(const char* src, size_t nbytes) { - const auto ret = ssl.write(socketDescriptor, src, gsl::narrow(nbytes)); - if (ssl.hasError() || ret == 0) { + // Start by trying to send everything we've already got + // buffered in our bio + ssl.drainBioSendPipe(socketDescriptor); + if (ssl.hasError()) { cb::net::set_econnreset(); return -1; } - if (ret > 0) { - return ret; - } - const int error = ssl.getError(ret); - if (error == SSL_ERROR_WANT_WRITE || error == SSL_ERROR_WANT_READ) { + // If the network socket is full there isn't much point + // of trying to add more to SSL + if (ssl.morePendingOutput()) { cb::net::set_ewouldblock(); return -1; } - // We encountered an error we don't have code to handle. - // Reset the connection - logSslErrorInfo("SSL_write", ret); - cb::net::set_econnreset(); - return -1; + // We've got an empty buffer for SSL to operate on, + // so lets get to it. + do { + const auto n = ssl.write(src, int(nbytes)); + if (n > 0) { + // we've successfully sent some bytes to + // SSL. Lets try to flush it to the network + // socket buffers. + ssl.drainBioSendPipe(socketDescriptor); + if (ssl.hasError()) { + cb::net::set_econnreset(); + return -1; + } + // Return the number of bytes submitted to SSL + return n; + } else if (n == 0) { + // Closed connection. + cb::net::set_econnreset(); + return -1; + } else { + const int error = ssl.getError(n); + if (error == SSL_ERROR_WANT_WRITE) { + ssl.drainBioSendPipe(socketDescriptor); + if (ssl.morePendingOutput()) { + cb::net::set_ewouldblock(); + return -1; + } + // We've got space in the network buffer. + // retry the operation (and openssl will + // try to fill the network buffer again) + } else { + // We enountered an error we don't have code + // to handle. Reset the connection + logSslErrorInfo("SSL_write", n); + cb::net::set_econnreset(); + return -1; + } + } + } while (true); } bool Connection::useCookieSendResponse(std::size_t size) const { diff --git a/daemon/ssl_context.h b/daemon/ssl_context.h index 13cf5b1775..664059ac56 100644 --- a/daemon/ssl_context.h +++ b/daemon/ssl_context.h @@ -107,18 +107,16 @@ class SslContext { * as possible. * * @param sfd the socket to read data from - * @return true if any progress was made */ - bool drainBioRecvPipe(SOCKET sfd); + void drainBioRecvPipe(SOCKET sfd); /** * Try to drain the SSL stream with as much data as possible and * send it over the network. * * @param sfd the socket to write data to - * @return true if any progress was made */ - bool drainBioSendPipe(SOCKET sfd); + void drainBioSendPipe(SOCKET sfd); bool moreInputAvailable() const { return !inputPipe.empty(); @@ -135,50 +133,13 @@ class SslContext { */ void dumpCipherList(uint32_t id) const; - /** - * Accept the next connection. - * - * Check hasError() before looking at the return code (hasError is for - * the underlying socket error) - * - * @param sfd The socket used for recv/send in the case we - * need more data - * @return 1 success - * 0 clean shutdown - * <0 an error - */ - int accept(SOCKET sfd); + int accept(); int getError(int errormask) const; - /** - * Read from the SSL pipe - * - * Check hasError() before looking at the return code (hasError is for - * the underlying socket error) - * - * @param sfd the underlying socket - * @param buf where to store the data - * @param num the amount of bytes to read - * @return > 0 - the number of bytes read - * <= 0 - an error occurred. Check with getError - */ - int read(SOCKET sfd, void* buf, int num); + int read(void* buf, int num); - /** - * Write data to the SSL pipe - * - * Check hasError() before looking at the return code (hasError is for - * the underlying socket error) - * - * @param sfd The underlying socket - * @param buf The data to send - * @param num The number of bytes ot send - * @return > 0 - The number of bytes sent - * 0 - Connection closed - * < 0 - An error occurred. Check with getError - */ - int write(SOCKET sfd, const void* buf, int num); + int write(const void* buf, int num); bool havePendingInputData(); @@ -192,16 +153,6 @@ class SslContext { const char* getCurrentCipherName() const; protected: - enum class DrainProgress { Error, Progress, NoProgress }; - - /** - * Try to drain the SSL output buffer and fill its input buffer - * - * @param sock the socket to use for transfer - * @return true if no error occurred - */ - DrainProgress drainBios(SOCKET sock); - bool drainInputSocketBuf(); bool enabled = false; diff --git a/daemon/ssl_context_openssl.cc b/daemon/ssl_context_openssl.cc index 4d82c752dd..87c65d5943 100644 --- a/daemon/ssl_context_openssl.cc +++ b/daemon/ssl_context_openssl.cc @@ -33,103 +33,20 @@ SslContext::~SslContext() { } } -int SslContext::accept(SOCKET sfd) { - do { - auto ret = SSL_accept(client); - if (ret > 0) { - return ret; - } - - if (ret == 0) { - return 0; - } - - const int e = getError(ret); - if (e == SSL_ERROR_WANT_WRITE || e == SSL_ERROR_WANT_READ) { - switch (drainBios(sfd)) { - case DrainProgress::Error: - return -1; - case DrainProgress::Progress: - // We made some progress... retry - break; - case DrainProgress::NoProgress: - return ret; - } - } else { - // We don't handle this error - return ret; - } - } while (true); +int SslContext::accept() { + return SSL_accept(client); } int SslContext::getError(int errormask) const { return SSL_get_error(client, errormask); } -int SslContext::read(SOCKET sfd, void* buf, int num) { - drainBioRecvPipe(sfd); - if (hasError()) { - return -1; - } - do { - auto ret = SSL_read(client, buf, num); - if (ret > 0) { - return ret; - } - - const int e = getError(ret); - if (e == SSL_ERROR_ZERO_RETURN) { - /* The TLS/SSL connection has been closed (cleanly). */ - return 0; - } else if (e == SSL_ERROR_WANT_WRITE || e == SSL_ERROR_WANT_READ) { - switch (drainBios(sfd)) { - case DrainProgress::Error: - return -1; - case DrainProgress::Progress: - // We made some progress... retry - break; - case DrainProgress::NoProgress: - return ret; - } - } else { - return ret; - } - } while (true); - // not reached +int SslContext::read(void* buf, int num) { + return SSL_read(client, buf, num); } -int SslContext::write(SOCKET sfd, const void* buf, int num) { - drainBioSendPipe(sfd); - if (hasError()) { - return -1; - } - do { - const auto ret = SSL_write(client, buf, num); - if (ret > 0) { - // try to drain the internal buffers in SSL and push them - // to the socket - drainBioSendPipe(sfd); - return ret; - } else if (ret == 0) { - return 0; - } else { - const int e = getError(ret); - if (e == SSL_ERROR_WANT_WRITE || e == SSL_ERROR_WANT_READ) { - switch (drainBios(sfd)) { - case DrainProgress::Error: - return -1; - case DrainProgress::Progress: - // We made some progress... retry - break; - case DrainProgress::NoProgress: - return ret; - } - } else { - return ret; - } - } - } while (true); - // not reached +int SslContext::write(const void* buf, int num) { + return SSL_write(client, buf, num); } bool SslContext::havePendingInputData() { @@ -247,18 +164,12 @@ bool SslContext::drainInputSocketBuf() { return false; } -bool SslContext::drainBioRecvPipe(SOCKET sfd) { - bool progress = false; +void SslContext::drainBioRecvPipe(SOCKET sfd) { bool stop; do { // Try to move data from our internal buffer to the SSL pipe - if (drainInputSocketBuf()) { - progress = true; - stop = false; - } else { - stop = true; - } + stop = !drainInputSocketBuf(); // If there is room in the input pipe (the internal buffer for read) // try to read out as much as possible from the socket @@ -273,7 +184,6 @@ bool SslContext::drainBioRecvPipe(SOCKET sfd) { totalRecv += n; // We did receive some data... move it into the BIO stop = false; - progress = true; } else { if (n == 0) { error = true; /* read end shutdown */ @@ -296,11 +206,9 @@ bool SslContext::drainBioRecvPipe(SOCKET sfd) { // * The BIO object is full and the input pipe is: // * full - there may be more data on the network // * not full - there is no more data to read from the network - return progress; } -bool SslContext::drainBioSendPipe(SOCKET sfd) { - bool progress = false; +void SslContext::drainBioSendPipe(SOCKET sfd) { bool stop; do { @@ -320,7 +228,6 @@ bool SslContext::drainBioSendPipe(SOCKET sfd) { totalSend += n; // We did move some data stop = false; - progress = true; } else { if (n == -1) { auto err = cb::net::get_socket_error(); @@ -331,7 +238,7 @@ bool SslContext::drainBioSendPipe(SOCKET sfd) { error = true; } } - return progress; + return; } } @@ -345,7 +252,6 @@ bool SslContext::drainBioSendPipe(SOCKET sfd) { if (n > 0) { // We did move data stop = false; - progress = true; } } // As long as we moved some data (from the internal buffer to the @@ -356,7 +262,6 @@ bool SslContext::drainBioSendPipe(SOCKET sfd) { // At this time there is: // * There is no more data to send // * The socket buffer is full - return progress; } void SslContext::dumpCipherList(uint32_t id) const { @@ -388,12 +293,3 @@ nlohmann::json SslContext::toJSON() const { const char* SslContext::getCurrentCipherName() const { return SSL_get_cipher_name(client); } - -SslContext::DrainProgress SslContext::drainBios(SOCKET sock) { - auto progress = (drainBioSendPipe(sock) || drainBioRecvPipe(sock)); - if (error) { - return DrainProgress::Error; - } - - return progress ? DrainProgress::Progress : DrainProgress::NoProgress; -}