File: revert_simplify_locking.diff

package info (click to toggle)
qtbase-opensource-src 5.15.18%2Bdfsg-2
  • links: PTS, VCS
  • area: main
  • in suites: experimental
  • size: 351,772 kB
  • sloc: cpp: 2,042,978; ansic: 402,848; xml: 126,413; python: 9,567; java: 7,541; asm: 4,023; sh: 2,093; perl: 2,047; yacc: 1,687; lex: 1,333; javascript: 878; makefile: 271; objc: 70
file content (183 lines) | stat: -rw-r--r-- 6,990 bytes parent folder | download | duplicates (2)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
Description: revert "QProcessEnvironment: simplify locking"
 This reverts commit c5d6b263c204cb09db2be36826e19acb03dc24fb.
 .
 The commit being reverted assumes the mutex is only protecting 'nameMap'
 and nothing else is mutable, which is false. The mutex is not only
 protecting 'nameMap' but also protecting the containing value objects,
 since even though the value object is accessed read-only, its
 implementation mutates its internal states for 2-way conversion between
 ByteArray and QString.
 .
 Commit 85e61297f7b02297641826332dbdbc845a88c34b ("restore
 QProcessEnvironment shared data thread safety on unix") said that
 implicit sharing together with 'mutable' is a time bomb and the bomb is
 triggered by the reverted commit.
Origin: upstream, https://code.qt.io/cgit/qt/qtbase.git/commit?id=080d61c020678b75
Last-Update: 2026-01-29

--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -202,7 +202,6 @@ void QProcessEnvironmentPrivate::insert(
         vars.insert(it.key(), it.value());
 
 #ifdef Q_OS_UNIX
-    const OrderedNameMapMutexLocker locker(this, &other);
     auto nit = other.nameMap.constBegin();
     const auto nend = other.nameMap.constEnd();
     for ( ; nit != nend; ++nit)
@@ -276,6 +275,7 @@ bool QProcessEnvironment::operator==(con
         return true;
     if (d) {
         if (other.d) {
+            QProcessEnvironmentPrivate::OrderedMutexLocker locker(d, other.d);
             return d->vars == other.d->vars;
         } else {
             return isEmpty();
@@ -322,6 +322,7 @@ bool QProcessEnvironment::contains(const
 {
     if (!d)
         return false;
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->vars.contains(d->prepareName(name));
 }
 
@@ -372,6 +373,7 @@ QString QProcessEnvironment::value(const
     if (!d)
         return defaultValue;
 
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
     const auto it = d->vars.constFind(d->prepareName(name));
     if (it == d->vars.constEnd())
         return defaultValue;
@@ -396,6 +398,7 @@ QStringList QProcessEnvironment::toStrin
 {
     if (!d)
         return QStringList();
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->toList();
 }
 
@@ -409,6 +412,7 @@ QStringList QProcessEnvironment::keys()
 {
     if (!d)
         return QStringList();
+    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->keys();
 }
 
@@ -425,6 +429,7 @@ void QProcessEnvironment::insert(const Q
         return;
 
     // our re-impl of detach() detaches from null
+    QProcessEnvironmentPrivate::MutexLocker locker(e.d);
     d->insert(*e.d);
 }
 
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -146,22 +146,16 @@ 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; }
-#else
-    struct NameMapMutexLocker : public QMutexLocker
-    {
-        NameMapMutexLocker(const QProcessEnvironmentPrivate *d) : QMutexLocker(&d->nameMapMutex) {}
+    struct MutexLocker {
+        MutexLocker(const QProcessEnvironmentPrivate *) {}
     };
-    struct OrderedNameMapMutexLocker : public QOrderedMutexLocker
-    {
-        OrderedNameMapMutexLocker(const QProcessEnvironmentPrivate *d1,
-                                  const QProcessEnvironmentPrivate *d2)
-            : QOrderedMutexLocker(&d1->nameMapMutex, &d2->nameMapMutex)
-        {}
+    struct OrderedMutexLocker {
+        OrderedMutexLocker(const QProcessEnvironmentPrivate *,
+                           const QProcessEnvironmentPrivate *) {}
     };
-
+#else
     inline Key prepareName(const QString &name) const
     {
-        const NameMapMutexLocker locker(this);
         Key &ent = nameMap[name];
         if (ent.isEmpty())
             ent = name.toLocal8Bit();
@@ -170,27 +164,40 @@ public:
     inline QString nameToString(const Key &name) const
     {
         const QString sname = QString::fromLocal8Bit(name);
-        {
-            const NameMapMutexLocker locker(this);
-            nameMap[sname] = name;
-        }
+        nameMap[sname] = name;
         return sname;
     }
     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(), vars(other.vars)
+        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()).
-        NameMapMutexLocker locker(&other);
+        MutexLocker locker(&other);
+        vars = other.vars;
         nameMap = other.nameMap;
-        // We need to detach our nameMap, so that our mutex can protect it.
-        // As we are being detached, it likely would be detached a moment later anyway.
+        // 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.
+        vars.detach();
         nameMap.detach();
     }
 #endif
@@ -201,7 +208,8 @@ public:
 #ifdef Q_OS_UNIX
     typedef QHash<QString, Key> NameHash;
     mutable NameHash nameMap;
-    mutable QMutex nameMapMutex;
+
+    mutable QMutex mutex;
 #endif
 
     static QProcessEnvironment fromList(const QStringList &list);
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -439,6 +439,7 @@ void QProcessPrivate::startProcess()
     int envc = 0;
     char **envp = nullptr;
     if (environment.d.constData()) {
+        QProcessEnvironmentPrivate::MutexLocker locker(environment.d);
         envp = _q_dupEnvironment(environment.d.constData()->vars, &envc);
     }
 
@@ -980,6 +981,7 @@ bool QProcessPrivate::startDetached(qint
             int envc = 0;
             char **envp = nullptr;
             if (environment.d.constData()) {
+                QProcessEnvironmentPrivate::MutexLocker locker(environment.d);
                 envp = _q_dupEnvironment(environment.d.constData()->vars, &envc);
             }