Fix race in colorspace LUT generation

The old code did not prevent concurrent writes to the LUTs by separate
threads, each finding lutsGenerated to be false.

Let's consider whether the change is safe now: the storeRelease(1)
comes before the QMutex::unlock(), but since it is release semantics
no writes may be ordered past it. We have two releases, and their
order doesn't matter, since nothing else happens in-between.

Could we use a normal relaxed store? No, because the unlock() of the
mutex only synchronizes with the lock() of the same mutex, which
doesn't happen if the loadAcquire() succeeds. For loadAcquire() to
happen-before a write to the luts, we need a storeRelease() on the
atomic.

So, everything is correct, and minimal.

But maybe, to save the next reader from having to do the same mental
exercise again, add a manual locker.unlock() in front of the
storeRelease()?

Again no: that opens a gap where the luts are already generated on T0,
and the mutex unlocked, but the atomic not set. If another thread T1
gets to execute the function, it will enter the critical section, then
writing new values to the LUT. Meanwhile, the T0 sets generate to true
and a T2 enters the function, sees the final write from T0 and starts
using the luts -> data race with the writes concurrently done by T1.

Change-Id: Id278812a74b6e326e3ddf0dbcbb94b34766aa52e
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>

src/gui/painting/qcolorspace.cpp

@@ -53,6 +53,8 @@
 
 QT_BEGIN_NAMESPACE
 
+QBasicMutex QColorSpacePrivate::s_lutWriteLock;
+
 QColorSpacePrimaries::QColorSpacePrimaries(QColorSpace::Gamut gamut)
 {
     switch (gamut) {

src/gui/painting/qcolorspace_p.h

@@ -56,8 +56,9 @@
 #include "qcolortrc_p.h"
 #include "qcolortrclut_p.h"
 
-#include <QtCore/qshareddata.h>
+#include <QtCore/qmutex.h>
 #include <QtCore/qpoint.h>
+#include <QtCore/qshareddata.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -112,8 +113,24 @@ public:
     QString description;
     QByteArray iccProfile;
 
-    mutable QSharedPointer<QColorTrcLut> lut[3];
-    mutable QAtomicInt lutsGenerated;
+    static QBasicMutex s_lutWriteLock;
+    struct LUT {
+        LUT() = default;
+        ~LUT() = default;
+        LUT(const LUT &other)
+        {
+            if (other.generated.loadAcquire()) {
+                table[0] = other.table[0];
+                table[1] = other.table[1];
+                table[2] = other.table[2];
+                generated.store(1);
+            }
+        }
+        QSharedPointer<QColorTrcLut> &operator[](int i) { return table[i]; }
+        const QSharedPointer<QColorTrcLut> &operator[](int i) const  { return table[i]; }
+        QSharedPointer<QColorTrcLut> table[3];
+        QAtomicInt generated;
+    } mutable lut;
 };
 
 QT_END_NAMESPACE

src/gui/painting/qcolortransform.cpp

@@ -68,8 +68,12 @@ QColorTrcLut *lutFromTrc(const QColorTrc &trc)
 
 void QColorTransformPrivate::updateLutsIn() const
 {
-    if (colorSpaceIn->lutsGenerated.loadAcquire())
+    if (colorSpaceIn->lut.generated.loadAcquire())
         return;
+    QMutexLocker lock(&QColorSpacePrivate::s_lutWriteLock);
+    if (colorSpaceIn->lut.generated.load())
+        return;
+
     for (int i = 0; i < 3; ++i) {
         if (!colorSpaceIn->trc[i].isValid())
             return;
@@ -84,12 +88,15 @@ void QColorTransformPrivate::updateLutsIn() const
             colorSpaceIn->lut[i].reset(lutFromTrc(colorSpaceIn->trc[i]));
     }
 
-    colorSpaceIn->lutsGenerated.storeRelease(1);
+    colorSpaceIn->lut.generated.storeRelease(1);
 }
 
 void QColorTransformPrivate::updateLutsOut() const
 {
-    if (colorSpaceOut->lutsGenerated.loadAcquire())
+    if (colorSpaceOut->lut.generated.loadAcquire())
+        return;
+    QMutexLocker lock(&QColorSpacePrivate::s_lutWriteLock);
+    if (colorSpaceOut->lut.generated.load())
         return;
     for (int i = 0; i < 3; ++i) {
         if (!colorSpaceOut->trc[i].isValid())
@@ -105,7 +112,7 @@ void QColorTransformPrivate::updateLutsOut() const
             colorSpaceOut->lut[i].reset(lutFromTrc(colorSpaceOut->trc[i]));
     }
 
-    colorSpaceOut->lutsGenerated.storeRelease(1);
+    colorSpaceOut->lut.generated.storeRelease(1);
 }
 
 /*!
@@ -150,7 +157,7 @@ QRgb QColorTransform::map(const QRgb &argb) const
     c.x = std::max(0.0f, std::min(1.0f, c.x));
     c.y = std::max(0.0f, std::min(1.0f, c.y));
     c.z = std::max(0.0f, std::min(1.0f, c.z));
-    if (d->colorSpaceOut->lutsGenerated.loadAcquire()) {
+    if (d->colorSpaceOut->lut.generated.loadAcquire()) {
         c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x);
         c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y);
         c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z);
@@ -182,7 +189,7 @@ QRgba64 QColorTransform::map(const QRgba64 &rgba64) const
     c.x = std::max(0.0f, std::min(1.0f, c.x));
     c.y = std::max(0.0f, std::min(1.0f, c.y));
     c.z = std::max(0.0f, std::min(1.0f, c.z));
-    if (d->colorSpaceOut->lutsGenerated.loadAcquire()) {
+    if (d->colorSpaceOut->lut.generated.loadAcquire()) {
         c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x);
         c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y);
         c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z);
@@ -221,7 +228,7 @@ QColor QColorTransform::map(const QColor &color) const
     c = d->colorMatrix.map(c);
     bool inGamut = c.x >= 0.0f && c.x <= 1.0f && c.y >= 0.0f && c.y <= 1.0f && c.z >= 0.0f && c.z <= 1.0f;
     if (inGamut) {
-        if (d_ptr->colorSpaceOut->lutsGenerated.loadAcquire()) {
+        if (d_ptr->colorSpaceOut->lut.generated.loadAcquire()) {
             c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x);
             c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y);
             c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z);