QCryptographicHash: fix UB (data race on concurrent result()) [2nd try]

The previous attempt at fixing QTBUG-110058,
ccad719d2e935306e601b0f6af5ff2acb7cd272e, was incomplete:

- the if (result.isEmpty()) check at the beginning of finalize() was
  not protected, so it raced against the assignment at the end of
  finalize(), which was protected

- because the mutex was not locked during the finalization of the hash
  algorithm, two threads could perform this operation simultaneously,
  which isn't such a bad idea in principle, as it can reduce latency,
  but for that to work, the losing thread needs to throw away its own
  work and adopt the work of the other thread, but that wasn't done:
  both threads would write their result to 'result', just one after
  the other, but that's still a data race, since the eventual _reader_
  of the result cannot be protected (is outside the class). Besides,
  we don't even know whether the algorithm-specific finalization
  functions are ok with being called from separate threads on the same
  context object

- in addition, the mutex wasn't necessary when finalize() was called
  from the static hash() function, as no sharing could possibly take
  place there (the state is function-local)

Fix all of the above by largely reverting the first attempt, dragging
the result.isEmpty() check out of finalize() and into resultView() and
instead simply holding the mutex over these two calls in
resultView(). To see why this is sufficient, consider that
resultView() is now idempotent again: the result is written once, the
next thread waits and then finds the work done. All following accesses
to the result are then reads, which happen-after the write at the end
of finalize().

The accesses to 'result' from reset() need no protection, as reset()
is a mutable function, and calling a mutable function on a shared
QCryptographicHash object is already UB. Only two const functions may
be called that way.

Manual conflict resolutions:
 - missing OpenSSLv3 code in 6.4
 - different order of Private member fields
 - Private::method mutable
 - no ctor in Private
 - no Private::reset(), so kept everything in result()
 - no static hash() optimization, so no non-locked finalize() there
 - as a consequence, just wrapped the whole body of result() in
   a scoped_lock.

Fixes: QTBUG-110058
Change-Id: Ia8ac095b785519682090801c1012e9dded6d60b2
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit b904de43a5acfc4067fc9e4146babd45c6ac1138)
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

1 files changed, 35 insertions, 36 deletions

diff --git a/src/corelib/tools/qcryptographichash.cpp b/src/corelib/tools/qcryptographichash.cpp
index e88acb6de7..29f1d2b93a 100644
--- a/src/corelib/tools/qcryptographichash.cpp
+++ b/src/corelib/tools/qcryptographichash.cpp
@@ -41,6 +41,7 @@
 #include <qcryptographichash.h>
 #include <qiodevice.h>
 #include <qmutex.h>
+#include <private/qlocking_p.h>
 
 #include "../../3rdparty/sha1/sha1.cpp"
 
@@ -163,15 +164,15 @@ public:
         Sha3,
         Keccak
     };
-    void sha3Finish(QByteArray *tmpresult, int bitCount, Sha3Variant sha3Variant);
+    void sha3Finish(int bitCount, Sha3Variant sha3Variant);
 #endif
+    // protects result in result()
     QBasicMutex finalizeMutex;
     QByteArray result;
 };
 
 #ifndef QT_CRYPTOGRAPHICHASH_ONLY_SHA1
-void QCryptographicHashPrivate::sha3Finish(QByteArray *tmpresult, int bitCount,
-                                           Sha3Variant sha3Variant)
+void QCryptographicHashPrivate::sha3Finish(int bitCount, Sha3Variant sha3Variant)
 {
     /*
         FIPS 202 §6.1 defines SHA-3 in terms of calculating the Keccak function
@@ -195,7 +196,7 @@ void QCryptographicHashPrivate::sha3Finish(QByteArray *tmpresult, int bitCount,
     */
     static const unsigned char sha3FinalSuffix = 0x80;
 
-    tmpresult->resize(bitCount / 8);
+    result.resize(bitCount / 8);
 
     SHA3Context copy = sha3Context;
 
@@ -207,7 +208,7 @@ void QCryptographicHashPrivate::sha3Finish(QByteArray *tmpresult, int bitCount,
         break;
     }
 
-    sha3Final(&copy, reinterpret_cast<BitSequence *>(tmpresult->data()));
+    sha3Final(&copy, reinterpret_cast<BitSequence *>(result.data()));
 }
 #endif
 
@@ -471,16 +472,18 @@ bool QCryptographicHash::addData(QIODevice *device)
 */
 QByteArray QCryptographicHash::result() const
 {
+    // result() is a const function, so concurrent calls are allowed; protect:
+    const auto lock = qt_scoped_lock(d->finalizeMutex);
+    // check that no other thread already finalized before us:
     if (!d->result.isEmpty())
         return d->result;
 
-    QByteArray tmpresult;
     switch (d->method) {
     case Sha1: {
         Sha1State copy = d->sha1Context;
-        tmpresult.resize(20);
+        d->result.resize(20);
         sha1FinalizeState(&copy);
-        sha1ToHash(&copy, (unsigned char *)tmpresult.data());
+        sha1ToHash(&copy, (unsigned char *)d->result.data());
         break;
     }
 #ifdef QT_CRYPTOGRAPHICHASH_ONLY_SHA1
@@ -491,70 +494,70 @@ QByteArray QCryptographicHash::result() const
 #else
     case Md4: {
         md4_context copy = d->md4Context;
-        tmpresult.resize(MD4_RESULTLEN);
-        md4_final(&copy, (unsigned char *)tmpresult.data());
+        d->result.resize(MD4_RESULTLEN);
+        md4_final(&copy, (unsigned char *)d->result.data());
         break;
     }
     case Md5: {
         MD5Context copy = d->md5Context;
-        tmpresult.resize(16);
-        MD5Final(&copy, (unsigned char *)tmpresult.data());
+        d->result.resize(16);
+        MD5Final(&copy, (unsigned char *)d->result.data());
         break;
     }
     case Sha224: {
         SHA224Context copy = d->sha224Context;
-        tmpresult.resize(SHA224HashSize);
-        SHA224Result(&copy, reinterpret_cast<unsigned char *>(tmpresult.data()));
+        d->result.resize(SHA224HashSize);
+        SHA224Result(&copy, reinterpret_cast<unsigned char *>(d->result.data()));
         break;
     }
     case Sha256: {
         SHA256Context copy = d->sha256Context;
-        tmpresult.resize(SHA256HashSize);
-        SHA256Result(&copy, reinterpret_cast<unsigned char *>(tmpresult.data()));
+        d->result.resize(SHA256HashSize);
+        SHA256Result(&copy, reinterpret_cast<unsigned char *>(d->result.data()));
         break;
     }
     case Sha384: {
         SHA384Context copy = d->sha384Context;
-        tmpresult.resize(SHA384HashSize);
-        SHA384Result(&copy, reinterpret_cast<unsigned char *>(tmpresult.data()));
+        d->result.resize(SHA384HashSize);
+        SHA384Result(&copy, reinterpret_cast<unsigned char *>(d->result.data()));
         break;
     }
     case Sha512: {
         SHA512Context copy = d->sha512Context;
-        tmpresult.resize(SHA512HashSize);
-        SHA512Result(&copy, reinterpret_cast<unsigned char *>(tmpresult.data()));
+        d->result.resize(SHA512HashSize);
+        SHA512Result(&copy, reinterpret_cast<unsigned char *>(d->result.data()));
         break;
     }
     case RealSha3_224: {
-        d->sha3Finish(&tmpresult, 224, QCryptographicHashPrivate::Sha3Variant::Sha3);
+        d->sha3Finish(224, QCryptographicHashPrivate::Sha3Variant::Sha3);
         break;
     }
     case RealSha3_256: {
-        d->sha3Finish(&tmpresult, 256, QCryptographicHashPrivate::Sha3Variant::Sha3);
+        d->sha3Finish(256, QCryptographicHashPrivate::Sha3Variant::Sha3);
         break;
     }
     case RealSha3_384: {
-        d->sha3Finish(&tmpresult, 384, QCryptographicHashPrivate::Sha3Variant::Sha3);
+        d->sha3Finish(384, QCryptographicHashPrivate::Sha3Variant::Sha3);
         break;
     }
     case RealSha3_512: {
-        d->sha3Finish(&tmpresult, 512, QCryptographicHashPrivate::Sha3Variant::Sha3);
+        d->sha3Finish(512, QCryptographicHashPrivate::Sha3Variant::Sha3);
         break;
     }
     case Keccak_224: {
-        d->sha3Finish(&tmpresult, 224, QCryptographicHashPrivate::Sha3Variant::Keccak);
+        d->sha3Finish(224, QCryptographicHashPrivate::Sha3Variant::Keccak);
         break;
     }
     case Keccak_256: {
-        d->sha3Finish(&tmpresult, 256, QCryptographicHashPrivate::Sha3Variant::Keccak);
+        d->sha3Finish(256, QCryptographicHashPrivate::Sha3Variant::Keccak);
         break;
     }
     case Keccak_384: {
-        d->sha3Finish(&tmpresult, 384, QCryptographicHashPrivate::Sha3Variant::Keccak);
+        d->sha3Finish(384, QCryptographicHashPrivate::Sha3Variant::Keccak);
         break;
     }
     case Keccak_512: {
-        d->sha3Finish(&tmpresult, 512, QCryptographicHashPrivate::Sha3Variant::Keccak);
+        d->sha3Finish(512, QCryptographicHashPrivate::Sha3Variant::Keccak);
         break;
     }
     case Blake2b_160:
@@ -563,8 +566,8 @@ QByteArray QCryptographicHash::result() const
     case Blake2b_512: {
         const auto length = hashLength(d->method);
         blake2b_state copy = d->blake2bContext;
-        tmpresult.resize(length);
-        blake2b_final(&copy, reinterpret_cast<uint8_t *>(tmpresult.data()), length);
+        d->result.resize(length);
+        blake2b_final(&copy, reinterpret_cast<uint8_t *>(d->result.data()), length);
         break;
     }
     case Blake2s_128:
@@ -573,17 +576,13 @@ QByteArray QCryptographicHash::result() const
     case Blake2s_256: {
         const auto length = hashLength(d->method);
         blake2s_state copy = d->blake2sContext;
-        tmpresult.resize(length);
-        blake2s_final(&copy, reinterpret_cast<uint8_t *>(tmpresult.data()), length);
+        d->result.resize(length);
+        blake2s_final(&copy, reinterpret_cast<uint8_t *>(d->result.data()), length);
         break;
     }
 #endif
     }
 
-    // we're called from a const function, so only write to this->result under
-    // a mutex
-    QMutexLocker locker(&d->finalizeMutex);
-    d->result = std::move(tmpresult);
     return d->result;
 }