From 69a43c6c3e3fcaf10480544c1638f6446ed25d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 27 Aug 2019 01:14:39 +0200 Subject: Schannel refactoring This moves some repeated code into functions (namely readToBuffer and retainExtraData) while also changing how the intermediateBuffer is handled to avoid deallocating and reallocating repeatedly. Change-Id: I49e6cee641f961565051a67123c56b1c8f3c0259 Reviewed-by: Edward Welbourne Reviewed-by: Timur Pocheptsov --- src/network/ssl/qsslsocket_schannel.cpp | 105 +++++++++++++++++++------------- 1 file changed, 61 insertions(+), 44 deletions(-) diff --git a/src/network/ssl/qsslsocket_schannel.cpp b/src/network/ssl/qsslsocket_schannel.cpp index d7fb080b49..3166cda4c9 100644 --- a/src/network/ssl/qsslsocket_schannel.cpp +++ b/src/network/ssl/qsslsocket_schannel.cpp @@ -431,6 +431,42 @@ QByteArray createAlpnString(const QByteArrayList &nextAllowedProtocols) return alpnString; } #endif // SUPPORTS_ALPN + +qint64 readToBuffer(QByteArray &buffer, QTcpSocket *plainSocket) +{ + Q_ASSERT(plainSocket); + static const qint64 shrinkCutoff = 1024 * 12; + static const qint64 defaultRead = 1024 * 16; + qint64 bytesRead = 0; + + const auto toRead = std::min(defaultRead, plainSocket->bytesAvailable()); + if (toRead > 0) { + const auto bufferSize = buffer.size(); + buffer.reserve(bufferSize + toRead); // avoid growth strategy kicking in + buffer.resize(bufferSize + toRead); + bytesRead = plainSocket->read(buffer.data() + bufferSize, toRead); + buffer.resize(bufferSize + bytesRead); + // In case of excessive memory usage we shrink: + if (buffer.size() < shrinkCutoff && buffer.capacity() > defaultRead) + buffer.shrink_to_fit(); + } + + return bytesRead; +} + +void retainExtraData(QByteArray &buffer, const SecBuffer &secBuffer) +{ + Q_ASSERT(secBuffer.BufferType == SECBUFFER_EXTRA); + if (int(secBuffer.cbBuffer) >= buffer.size()) + return; + +#ifdef QSSLSOCKET_DEBUG + qCDebug(lcSsl, "We got SECBUFFER_EXTRA, will retain %lu bytes", secBuffer.cbBuffer); +#endif + std::move(buffer.end() - secBuffer.cbBuffer, buffer.end(), buffer.begin()); + buffer.resize(secBuffer.cbBuffer); +} + } // anonymous namespace bool QSslSocketPrivate::s_loadRootCertsOnDemand = true; @@ -619,8 +655,8 @@ bool QSslSocketBackendPrivate::acquireCredentialsHandle() nullptr); if (!chainContext) { const QString message = isClient - ? QSslSocket::tr("The certificate provided cannot be used for a client.") - : QSslSocket::tr("The certificate provided cannot be used for a server."); + ? QSslSocket::tr("The certificate provided cannot be used for a client.") + : QSslSocket::tr("The certificate provided cannot be used for a server."); setErrorAndEmit(QAbstractSocket::SocketError::SslInvalidUserDataError, message); return false; } @@ -774,7 +810,7 @@ bool QSslSocketBackendPrivate::acceptContext() Q_ASSERT(mode == QSslSocket::SslServerMode); ULONG contextReq = getContextRequirements(); - intermediateBuffer += plainSocket->read(16384); + readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // definitely need more data.. @@ -837,9 +873,9 @@ bool QSslSocketBackendPrivate::acceptContext() // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // inBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to // be stored. - intermediateBuffer = intermediateBuffer.right(int(inBuffers[1].cbBuffer)); + retainExtraData(intermediateBuffer, inBuffers[1]); } else { /* No 'extra' data, message not incomplete */ - intermediateBuffer.clear(); + intermediateBuffer.resize(0); } if (status != SEC_I_CONTINUE_NEEDED) { @@ -865,11 +901,11 @@ bool QSslSocketBackendPrivate::performHandshake() Q_ASSERT(schannelState == SchannelState::PerformHandshake); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Bytes available from socket:" << plainSocket->bytesAvailable(); - qCDebug(lcSsl) << "intermediateBuffer size:" << intermediateBuffer.size(); + qCDebug(lcSsl, "Bytes available from socket: %lld", plainSocket->bytesAvailable()); + qCDebug(lcSsl, "intermediateBuffer size: %d", intermediateBuffer.size()); #endif - intermediateBuffer += plainSocket->read(16384); + readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // no data, will fail @@ -918,11 +954,10 @@ bool QSslSocketBackendPrivate::performHandshake() // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // inputBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to // be stored. - intermediateBuffer = intermediateBuffer.right(int(inputBuffers[1].cbBuffer)); - } else { + retainExtraData(intermediateBuffer, inputBuffers[1]); + } else if (status != SEC_E_INCOMPLETE_MESSAGE) { // Clear the buffer if we weren't asked for more data - if (status != SEC_E_INCOMPLETE_MESSAGE) - intermediateBuffer.clear(); + intermediateBuffer.resize(0); } switch (status) { case SEC_E_OK: @@ -1228,8 +1263,7 @@ void QSslSocketBackendPrivate::transmit() fullMessage.resize(inputBuffers[0].cbBuffer + inputBuffers[1].cbBuffer + inputBuffers[2].cbBuffer); const qint64 bytesWritten = plainSocket->write(fullMessage); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Wrote" << bytesWritten << "of total" - << fullMessage.length() << "bytes"; + qCDebug(lcSsl, "Wrote %lld of total %d bytes", bytesWritten, fullMessage.length()); #endif if (bytesWritten >= 0) { totalBytesWritten += bytesWritten; @@ -1254,36 +1288,24 @@ void QSslSocketBackendPrivate::transmit() int totalRead = 0; bool hadIncompleteData = false; while (!readBufferMaxSize || buffer.size() < readBufferMaxSize) { - QByteArray ciphertext; - if (intermediateBuffer.length()) { + const qint64 bytesRead = readToBuffer(intermediateBuffer, plainSocket); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Restoring data from intermediateBuffer:" - << intermediateBuffer.length() << "bytes"; + qCDebug(lcSsl, "Read %lld encrypted bytes from the socket", bytesRead); #endif - ciphertext.swap(intermediateBuffer); - } - int initialLength = ciphertext.length(); - ciphertext += plainSocket->read(16384); + if (intermediateBuffer.length() == 0 || (hadIncompleteData && bytesRead == 0)) { #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Read" << ciphertext.length() - initialLength - << "encrypted bytes from the socket"; + qCDebug(lcSsl, (hadIncompleteData ? "No new data received, leaving loop!" + : "Nothing to decrypt, leaving loop!")); #endif - if (ciphertext.length() == 0 || (hadIncompleteData && initialLength == ciphertext.length())) { -#ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << (hadIncompleteData ? "No new data received, leaving loop!" - : "Nothing to decrypt, leaving loop!"); -#endif - if (ciphertext.length()) // We have data, it came from intermediateBuffer, swap back - intermediateBuffer.swap(ciphertext); break; } hadIncompleteData = false; #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Total amount of bytes to decrypt:" << ciphertext.length(); + qCDebug(lcSsl, "Total amount of bytes to decrypt: %d", intermediateBuffer.length()); #endif SecBuffer dataBuffer[4]{ - createSecBuffer(ciphertext, SECBUFFER_DATA), + createSecBuffer(intermediateBuffer, SECBUFFER_DATA), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY) @@ -1299,34 +1321,29 @@ void QSslSocketBackendPrivate::transmit() if (dataBuffer[1].cbBuffer > 0) { // It is always decrypted in-place. // But [0] is the STREAM_HEADER, [1] is the DATA and [2] is the STREAM_TRAILER. - // The pointers in all of those still point into the 'ciphertext' byte array. + // The pointers in all of those still point into 'intermediateBuffer'. buffer.append(static_cast(dataBuffer[1].pvBuffer), dataBuffer[1].cbBuffer); totalRead += dataBuffer[1].cbBuffer; #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Decrypted" << dataBuffer[1].cbBuffer - << "bytes. New read buffer size:" << buffer.size(); + qCDebug(lcSsl, "Decrypted %lu bytes. New read buffer size: %d", + dataBuffer[1].cbBuffer, buffer.size()); #endif } if (dataBuffer[3].BufferType == SECBUFFER_EXTRA) { // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // dataBuffer[3].cbBuffer indicates the amount of bytes _NOT_ processed, // the rest need to be stored. -#ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "We've got excess data, moving it to the intermediate buffer:" - << dataBuffer[3].cbBuffer << "bytes"; -#endif - intermediateBuffer = ciphertext.right(int(dataBuffer[3].cbBuffer)); + retainExtraData(intermediateBuffer, dataBuffer[3]); + } else { + intermediateBuffer.resize(0); } } if (status == SEC_E_INCOMPLETE_MESSAGE) { - // Need more data before we can decrypt.. to the buffer it goes! #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl, "We didn't have enough data to decrypt anything, will try again!"); #endif - Q_ASSERT(intermediateBuffer.isEmpty()); - intermediateBuffer.swap(ciphertext); // We try again, but if we don't get any more data then we leave hadIncompleteData = true; } else if (status == SEC_E_INVALID_HANDLE) { -- cgit v1.2.3