Fix crash in parallel YAML parsing

QRegExp is not thread-safe, but we are using it in multiple threads when
parsing config files on startup.
Backported the 5.14 implementation and added docs on why it works now.
Also backported and extended the 5.14 autotest.

Change-Id: I09d93b9783c8af4dffe0b3b37efc0754d293c955
Fixes: AUTOSUITE-1410
Reviewed-by: Bernd Weimer <bernd.weimer@pelagicore.com>

5 files changed, 244 insertions, 16 deletions

diff --git a/src/common-lib/qtyaml.cpp b/src/common-lib/qtyaml.cpp
index fbb75f50..509f106a 100644
--- a/src/common-lib/qtyaml.cpp
+++ b/src/common-lib/qtyaml.cpp
@@ -41,7 +41,7 @@
 ****************************************************************************/
 
 #include <QVariant>
-#include <QRegExp>
+#include <QRegularExpression>
 #include <QDebug>
 #include <QtNumeric>
 
@@ -156,17 +156,21 @@ static QVariant convertYamlNodeToVariant(yaml_document_t *doc, yaml_node_t *node
 
         if ((firstChar >= '0' && firstChar <= '9')   // cheap check to avoid expensive regexps
                 || firstChar == '+' || firstChar == '-' || firstChar == '.') {
-            static const QRegExp numberRegExps[] = {
-                QRegExp(qSL("[-+]?0b[0-1_]+")),        // binary
-                QRegExp(qSL("[-+]?0x[0-9a-fA-F_]+")),  // hexadecimal
-                QRegExp(qSL("[-+]?0[0-7_]+")),         // octal
-                QRegExp(qSL("[-+]?(0|[1-9][0-9_]*)")), // decimal
-                QRegExp(qSL("[-+]?([0-9][0-9_]*)?\\.[0-9.]*([eE][-+][0-9]+)?")), // float
-                QRegExp()
+            // We are using QRegularExpressions in multiple threads here, although the class is not
+            // marked thread-safe. We are relying on the const match() function to behave thread-safe
+            // which it does.
+            // The easiest way would be to deep-copy the objects into TLS instances, but
+            // QRegularExpression is lacking such a functionality. Creating all the objects from
+            // scratch in every thread is expensive though, so we count on match() being thread-safe.
+            static const QRegularExpression numberRegExps[] = {
+                QRegularExpression(qSL("\\A[-+]?(0|[1-9][0-9_]*)\\z")), // decimal
+                QRegularExpression(qSL("\\A[-+]?([0-9][0-9_]*)?\\.[0-9.]*([eE][-+][0-9]+)?\\z")), // float
+                QRegularExpression(qSL("\\A[-+]?0x[0-9a-fA-F_]+\\z")),  // hexadecimal
+                QRegularExpression(qSL("\\A[-+]?0b[0-1_]+\\z")),        // binary
+                QRegularExpression(qSL("\\A[-+]?0[0-7_]+\\z")),         // octal
             };
-
-            for (int numberIndex = 0; !numberRegExps[numberIndex].isEmpty(); ++numberIndex) {
-                if (numberRegExps[numberIndex].exactMatch(str)) {
+            for (size_t numberIndex = 0; numberIndex < (sizeof(numberRegExps) / sizeof(*numberRegExps)); ++numberIndex) {
+                if (numberRegExps[numberIndex].match(str).hasMatch()) {
                     bool ok = false;
                     QVariant val;
 
@@ -174,16 +178,16 @@ static QVariant convertYamlNodeToVariant(yaml_document_t *doc, yaml_node_t *node
                     if (str.contains(qL1C('_')))
                         str = str.replace(qL1C('_'), qSL(""));
 
-                    if (numberIndex == 4) {
+                    if (numberIndex == 1) {
                         val = str.toDouble(&ok);
                     } else {
                         int base = 10;
 
                         switch (numberIndex) {
-                        case 0: base = 2; str.replace(qSL("0b"), qSL("")); break; // Qt chokes on 0b
-                        case 1: base = 16; break;
-                        case 2: base = 8; break;
-                        case 3: base = 10; break;
+                        case 0: base = 10; break;
+                        case 2: base = 16; break;
+                        case 3: base = 2; str.replace(qSL("0b"), qSL("")); break; // Qt chokes on 0b
+                        case 4: base = 8; break;
                         }
 
                         qint64 s64 = str.toLongLong(&ok, base);
diff --git a/tests/tests.pro b/tests/tests.pro
index 0b9409df..0178f903 100644
--- a/tests/tests.pro
+++ b/tests/tests.pro
@@ -15,6 +15,7 @@ SUBDIRS = \
     applicationinstaller \
     debugwrapper \
     qml \
+    yaml \
 
 linux*:SUBDIRS += \
     sudo \
diff --git a/tests/yaml/data/test.yaml b/tests/yaml/data/test.yaml
new file mode 100644
index 00000000..b16e3d39
--- /dev/null
+++ b/tests/yaml/data/test.yaml
@@ -0,0 +1,45 @@
+formatVersion: 42
+formatType: testfile
+---
+dec: 10
+hex: 0x10
+oct: 010
+bin: 0b10
+float1: 10.10
+float2: 0.10
+float3: .10
+number-separators: 1_234_567
+bool-true: true
+bool-yes: yes
+bool-false: false
+bool-no: no
+null-literal: null
+null-tilde: ~
+string-unquoted: unquoted
+string-singlequoted: 'singlequoted'
+string-doublequoted: "doublequoted"
+list-int:
+- 1
+- 2
+- 3
+list-mixed:
+- 1
+- two
+- [yes, ~]
+map1:
+  a: 1
+  b: two
+  c: [ 1, 2, 3]
+
+extended:
+  ext-string: 'ext string'
+
+stringlist-string: string
+stringlist-list1: [ string ]
+stringlist-list2: [ string1, string2 ]
+
+list-of-maps:
+- index: 1
+  name: '1'
+- index: 2
+  name: '2'
diff --git a/tests/yaml/tst_yaml.cpp b/tests/yaml/tst_yaml.cpp
new file mode 100644
index 00000000..e62e4f26
--- /dev/null
+++ b/tests/yaml/tst_yaml.cpp
@@ -0,0 +1,168 @@
+/****************************************************************************
+**
+** Copyright (C) 2019 The Qt Company Ltd.
+** Copyright (C) 2019 Luxoft Sweden AB
+** Copyright (C) 2018 Pelagicore AG
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the Qt Application Manager.
+**
+** $QT_BEGIN_LICENSE:GPL-EXCEPT-QTAS$
+** Commercial License Usage
+** Licensees holding valid commercial Qt Automotive Suite licenses may use
+** this file in accordance with the commercial license agreement provided
+** with the Software or, alternatively, in accordance with the terms
+** contained in a written agreement between you and The Qt Company.  For
+** licensing terms and conditions see https://www.qt.io/terms-conditions.
+** For further information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include <QtCore>
+#include <QtTest>
+#include <QThreadPool>
+
+#include "qtyaml.h"
+#include "exception.h"
+#include "global.h"
+
+QT_USE_NAMESPACE_AM
+
+class tst_Yaml : public QObject
+{
+    Q_OBJECT
+
+public:
+    tst_Yaml();
+
+private slots:
+    void documentParser();
+    void parallel();
+};
+
+
+tst_Yaml::tst_Yaml()
+{ }
+
+static const QVariant vnull = QVariant();
+
+static const QVariantMap testHeaderDoc = {
+    { "formatVersion", 42 }, { "formatType", "testfile" }
+};
+
+static const QVariantMap testMainDoc = {
+    { "dec", 10 },
+    { "hex", 16 },
+    { "bin", 2 },
+    { "oct", 8 },
+    { "float1", 10.1 },
+    { "float2", .1 },
+    { "float3", .1 },
+    { "number-separators", 1234567 },
+    { "bool-true", true },
+    { "bool-yes", true },
+    { "bool-false", false },
+    { "bool-no", false },
+    { "null-literal", vnull },
+    { "null-tilde", vnull },
+    { "string-unquoted", "unquoted" },
+    { "string-singlequoted", "singlequoted" },
+    { "string-doublequoted", "doublequoted" },
+    { "list-int", QVariantList { 1, 2, 3 } },
+    { "list-mixed", QVariantList { 1, qSL("two"), QVariantList { true, vnull } } },
+    { "map1", QVariantMap { { "a", 1 }, { "b", "two" }, { "c", QVariantList { 1, 2, 3 } } } },
+
+
+    { "extended", QVariantMap { { "ext-string", "ext string" } } },
+
+    { "stringlist-string", "string" },
+    { "stringlist-list1", QVariantList { "string" } },
+    { "stringlist-list2", QVariantList { "string1", "string2" } },
+
+    { "list-of-maps", QVariantList { QVariantMap { { "index", 1 }, { "name", "1" } },
+                                     QVariantMap { { "index", 2 }, { "name", "2" } } } }
+};
+
+void tst_Yaml::documentParser()
+{
+    try {
+        QFile f(":/data/test.yaml");
+        QVERIFY2(f.open(QFile::ReadOnly), qPrintable(f.errorString()));
+        QByteArray ba = f.readAll();
+        QVERIFY(!ba.isEmpty());
+        QVector<QVariant> docs = QtYaml::variantDocumentsFromYaml(ba);
+        QCOMPARE(docs.size(), 2);
+        QCOMPARE(docs.at(0).toMap().size(), 2);
+
+        QCOMPARE(testHeaderDoc, docs.at(0).toMap());
+        QCOMPARE(testMainDoc, docs.at(1).toMap());
+
+    } catch (const Exception &e) {
+        QVERIFY2(false, e.what());
+    }
+}
+
+class YamlRunnable : public QRunnable
+{
+public:
+    YamlRunnable(const QByteArray &yaml, QAtomicInt &success, QAtomicInt &fail)
+        : m_yaml(yaml)
+        , m_success(success)
+        , m_fail(fail)
+    { }
+
+    void run() override
+    {
+        QVector<QVariant> docs = QtYaml::variantDocumentsFromYaml(m_yaml);
+        if ((docs.size() == 2)
+            && (docs.at(0).toMap().size() == 2)
+            && (testHeaderDoc == docs.at(0).toMap())
+            && (testMainDoc == docs.at(1).toMap())) {
+            m_success.fetchAndAddOrdered(1);
+        } else {
+            m_fail.fetchAndAddOrdered(1);
+        }
+    }
+private:
+    const QByteArray m_yaml;
+    QAtomicInt &m_success;
+    QAtomicInt &m_fail;
+};
+
+void tst_Yaml::parallel()
+{
+    QFile f(":/data/test.yaml");
+    QVERIFY2(f.open(QFile::ReadOnly), qPrintable(f.errorString()));
+    QByteArray ba = f.readAll();
+    QVERIFY(!ba.isEmpty());
+
+    constexpr int threadCount = 16;
+
+    QAtomicInt success;
+    QAtomicInt fail;
+
+    QThreadPool tp;
+    if (tp.maxThreadCount() < threadCount)
+        tp.setMaxThreadCount(threadCount);
+
+    for (int i = 0; i < threadCount; ++i)
+        tp.start(new YamlRunnable(ba, success, fail));
+
+    QVERIFY(tp.waitForDone(5000));
+    QCOMPARE(fail.loadAcquire(), 0);
+    QCOMPARE(success.loadAcquire(), threadCount);
+}
+
+QTEST_MAIN(tst_Yaml)
+
+#include "tst_yaml.moc"
diff --git a/tests/yaml/yaml.pro b/tests/yaml/yaml.pro
new file mode 100644
index 00000000..26ebb5d5
--- /dev/null
+++ b/tests/yaml/yaml.pro
@@ -0,0 +1,10 @@
+TARGET = tst_yaml
+
+include($$PWD/../tests.pri)
+
+QT *= appman_common-private
+
+SOURCES += tst_yaml.cpp
+
+RESOURCES += \
+    data/test.yaml \