restore QProcessEnvironment shared data thread safety on unix

implicit sharing together with 'mutable' is a time bomb.

we need to protect the nameMap, because concurrent "reads" may try to
insert into the hash, which would go boom.
we need to protect the key/value of Hash objects, because while the
refcounting is atomic, the d pointer assignments are not, which would
also go boom.

we can simply use a QMutex to protect the whole environment, because it
is very cheap in the uncontended case.

Task-number: QTBUG-30779
Change-Id: Iaad5720041ca06691d75eb9c6c0e1c120d4a7b46
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>

3 files changed, 74 insertions, 8 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 6ff43ab943..b1861d8038 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -265,7 +265,13 @@ QProcessEnvironment &QProcessEnvironment::operator=(const QProcessEnvironment &o
 */
 bool QProcessEnvironment::operator==(const QProcessEnvironment &other) const
 {
-    return d == other.d || (d && other.d && d->hash == other.d->hash);
+    if (d == other.d)
+        return true;
+    if (d && other.d) {
+        QProcessEnvironmentPrivate::OrderedMutexLocker locker(d, other.d);
+        return d->hash == other.d->hash;
+    }
+    return false;
 }
 
 /*!
@@ -276,6 +282,7 @@ bool QProcessEnvironment::operator==(const QProcessEnvironment &other) const
 */
 bool QProcessEnvironment::isEmpty() const
 {
+    // Needs no locking, as no hash nodes are accessed
     return d ? d->hash.isEmpty() : true;
 }
 
@@ -302,7 +309,10 @@ void QProcessEnvironment::clear()
 */
 bool QProcessEnvironment::contains(const QString &name) const
 {
-    return d ? d->hash.contains(d->prepareName(name)) : false;
+    if (!d)
+        return false;
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
+    return d->hash.contains(d->prepareName(name));
 }
 
 /*!
@@ -319,7 +329,8 @@ bool QProcessEnvironment::contains(const QString &name) const
 */
 void QProcessEnvironment::insert(const QString &name, const QString &value)
 {
-    // d detaches from null
+    // our re-impl of detach() detaches from null
+    d.detach(); // detach before prepareName()
     d->hash.insert(d->prepareName(name), d->prepareValue(value));
 }
 
@@ -333,8 +344,10 @@ void QProcessEnvironment::insert(const QString &name, const QString &value)
 */
 void QProcessEnvironment::remove(const QString &name)
 {
-    if (d)
+    if (d) {
+        d.detach(); // detach before prepareName()
         d->hash.remove(d->prepareName(name));
+    }
 }
 
 /*!
@@ -349,6 +362,7 @@ QString QProcessEnvironment::value(const QString &name, const QString &defaultVa
     if (!d)
         return defaultValue;
 
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
     QProcessEnvironmentPrivate::Hash::ConstIterator it = d->hash.constFind(d->prepareName(name));
     if (it == d->hash.constEnd())
         return defaultValue;
@@ -371,7 +385,10 @@ QString QProcessEnvironment::value(const QString &name, const QString &defaultVa
 */
 QStringList QProcessEnvironment::toStringList() const
 {
-    return d ? d->toList() : QStringList();
+    if (!d)
+        return QStringList();
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
+    return d->toList();
 }
 
 /*!
@@ -382,7 +399,10 @@ QStringList QProcessEnvironment::toStringList() const
 */
 QStringList QProcessEnvironment::keys() const
 {
-    return d ? d->keys() : QStringList();
+    if (!d)
+        return QStringList();
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
+    return d->keys();
 }
 
 /*!
@@ -397,7 +417,8 @@ void QProcessEnvironment::insert(const QProcessEnvironment &e)
     if (!e.d)
         return;
 
-    // d detaches from null
+    // our re-impl of detach() detaches from null
+    QProcessEnvironmentPrivate::MutexLocker locker(e.d);
     d->insert(*e.d);
 }
 
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index bd6943c8d0..2a2cc9fb84 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -59,6 +59,9 @@
 #include "QtCore/qshareddata.h"
 #include "private/qringbuffer_p.h"
 #include "private/qiodevice_p.h"
+#ifdef Q_OS_UNIX
+#include <QtCore/private/qorderedmutexlocker_p.h>
+#endif
 
 #ifdef Q_OS_WIN
 #include "QtCore/qt_windows.h"
@@ -148,6 +151,13 @@ public:
     inline QString nameToString(const Key &name) const { return name; }
     inline Value prepareValue(const QString &value) const { return value; }
     inline QString valueToString(const Value &value) const { return value; }
+    struct MutexLocker {
+        MutexLocker(const QProcessEnvironmentPrivate *) {}
+    };
+    struct OrderedMutexLocker {
+        OrderedMutexLocker(const QProcessEnvironmentPrivate *,
+                           const QProcessEnvironmentPrivate *) {}
+    };
 #else
     inline Key prepareName(const QString &name) const
     {
@@ -164,6 +174,37 @@ public:
     }
     inline Value prepareValue(const QString &value) const { return Value(value); }
     inline QString valueToString(const Value &value) const { return value.string(); }
+
+    struct MutexLocker : public QMutexLocker
+    {
+        MutexLocker(const QProcessEnvironmentPrivate *d) : QMutexLocker(&d->mutex) {}
+    };
+    struct OrderedMutexLocker : public QOrderedMutexLocker
+    {
+        OrderedMutexLocker(const QProcessEnvironmentPrivate *d1,
+                           const QProcessEnvironmentPrivate *d2) :
+            QOrderedMutexLocker(&d1->mutex, &d2->mutex)
+        {}
+    };
+
+    QProcessEnvironmentPrivate() : QSharedData() {}
+    QProcessEnvironmentPrivate(const QProcessEnvironmentPrivate &other) :
+        QSharedData()
+    {
+        // This being locked ensures that the functions that only assign
+        // d pointers don't need explicit locking.
+        // We don't need to lock our own mutex, as this object is new and
+        // consequently not shared. For the same reason, non-const methods
+        // do not need a lock, as they detach objects (however, we need to
+        // ensure that they really detach before using prepareName()).
+        MutexLocker locker(&other);
+        hash = other.hash;
+        nameMap = other.nameMap;
+        // We need to detach our members, so that our mutex can protect them.
+        // As we are being detached, they likely would be detached a moment later anyway.
+        hash.detach();
+        nameMap.detach();
+    }
 #endif
 
     typedef QHash<Key, Value> Hash;
@@ -172,6 +213,8 @@ public:
 #ifdef Q_OS_UNIX
     typedef QHash<QString, Key> NameHash;
     mutable NameHash nameMap;
+
+    mutable QMutex mutex;
 #endif
 
     static QProcessEnvironment fromList(const QStringList &list);
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index 79a75b0321..e9957d2384 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -617,8 +617,10 @@ void QProcessPrivate::startProcess()
     // Duplicate the environment.
     int envc = 0;
     char **envp = 0;
-    if (environment.d.constData())
+    if (environment.d.constData()) {
+        QProcessEnvironmentPrivate::MutexLocker locker(environment.d);
         envp = _q_dupEnvironment(environment.d.constData()->hash, &envc);
+    }
 
     // Encode the working directory if it's non-empty, otherwise just pass 0.
     const char *workingDirPtr = 0;