From 67b091c49cafb607db9756333aaa7229817f7348 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Thu, 15 Mar 2018 17:08:21 +0100 Subject: Fix crash when calling QQmlEngine::clearComponentCache() We must protect various resources in the type loader with our existing lock. The QQmlTypeLoaderQmldirContent is now value based, so that we can release the lock on the shared cache early. Copying it involves adjusting the refcount of the QHash and QString instances in the QQmlDirParser. The safety of this was verified with a TSAN build and the example supplied in the task. It crashed reliably with TASN errors first and with this patch it runs without errors. Task-number: QTBUG-41465 Change-Id: I616843c4b8bdfd65d1277d4faa8cb884d8e77df8 Reviewed-by: Lars Knoll (cherry picked from commit a3ad52526f79c1528f170c8affe5af00b68ca61d) --- src/qml/qml/qqmldirparser_p.h | 2 -- src/qml/qml/qqmlengine.cpp | 2 ++ src/qml/qml/qqmlimport.cpp | 62 +++++++++++++++++++++--------------------- src/qml/qml/qqmlimport_p.h | 2 +- src/qml/qml/qqmltypeloader.cpp | 55 ++++++++++++++++++++----------------- src/qml/qml/qqmltypeloader_p.h | 9 ++++-- 6 files changed, 71 insertions(+), 61 deletions(-) diff --git a/src/qml/qml/qqmldirparser_p.h b/src/qml/qml/qqmldirparser_p.h index 1530b7a6cf..1297d21fa0 100644 --- a/src/qml/qml/qqmldirparser_p.h +++ b/src/qml/qml/qqmldirparser_p.h @@ -63,8 +63,6 @@ class QQmlError; class QQmlEngine; class Q_QML_PRIVATE_EXPORT QQmlDirParser { - Q_DISABLE_COPY(QQmlDirParser) - public: QQmlDirParser(); ~QQmlDirParser(); diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp index f4656bafd2..ac92f8dde8 100644 --- a/src/qml/qml/qqmlengine.cpp +++ b/src/qml/qml/qqmlengine.cpp @@ -1098,7 +1098,9 @@ QQmlEngine::~QQmlEngine() void QQmlEngine::clearComponentCache() { Q_D(QQmlEngine); + d->typeLoader.lock(); d->typeLoader.clearCache(); + d->typeLoader.unlock(); } /*! diff --git a/src/qml/qml/qqmlimport.cpp b/src/qml/qml/qqmlimport.cpp index f43ffb4c3c..6e467a6d96 100644 --- a/src/qml/qml/qqmlimport.cpp +++ b/src/qml/qml/qqmlimport.cpp @@ -316,17 +316,17 @@ public: QQmlImportDatabase *database, QString *outQmldirFilePath, QString *outUrl); - static bool validateQmldirVersion(const QQmlTypeLoaderQmldirContent *qmldir, const QString &uri, int vmaj, int vmin, + static bool validateQmldirVersion(const QQmlTypeLoaderQmldirContent &qmldir, const QString &uri, int vmaj, int vmin, QList *errors); bool importExtension(const QString &absoluteFilePath, const QString &uri, int vmaj, int vmin, QQmlImportDatabase *database, - const QQmlTypeLoaderQmldirContent *qmldir, + const QQmlTypeLoaderQmldirContent &qmldir, QList *errors); bool getQmldirContent(const QString &qmldirIdentifier, const QString &uri, - const QQmlTypeLoaderQmldirContent **qmldir, QList *errors); + QQmlTypeLoaderQmldirContent *qmldir, QList *errors); QString resolvedUri(const QString &dir_arg, QQmlImportDatabase *database); @@ -664,14 +664,14 @@ bool QQmlImports::resolveType(const QHashedStringRef &type, return false; } -bool QQmlImportInstance::setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent *qmldir, QQmlImportNamespace *nameSpace, QList *errors) +bool QQmlImportInstance::setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent &qmldir, QQmlImportNamespace *nameSpace, QList *errors) { Q_ASSERT(resolvedUrl.endsWith(Slash)); url = resolvedUrl; - qmlDirComponents = qmldir->components(); + qmlDirComponents = qmldir.components(); - const QQmlDirScripts &scripts = qmldir->scripts(); + const QQmlDirScripts &scripts = qmldir.scripts(); if (!scripts.isEmpty()) { // Verify that we haven't imported these scripts already for (QList::const_iterator it = nameSpace->imports.constBegin(); @@ -1060,26 +1060,26 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath, const QString &uri, int vmaj, int vmin, QQmlImportDatabase *database, - const QQmlTypeLoaderQmldirContent *qmldir, + const QQmlTypeLoaderQmldirContent &qmldir, QList *errors) { - Q_ASSERT(qmldir); + Q_ASSERT(qmldir.hasContent()); if (qmlImportTrace()) qDebug().nospace() << "QQmlImports(" << qPrintable(base) << ")::importExtension: " << "loaded " << qmldirFilePath; - if (designerSupportRequired && !qmldir->designerSupported()) { + if (designerSupportRequired && !qmldir.designerSupported()) { if (errors) { QQmlError error; - error.setDescription(QQmlImportDatabase::tr("module does not support the designer \"%1\"").arg(qmldir->typeNamespace())); + error.setDescription(QQmlImportDatabase::tr("module does not support the designer \"%1\"").arg(qmldir.typeNamespace())); error.setUrl(QUrl::fromLocalFile(qmldirFilePath)); errors->prepend(error); } return false; } - int qmldirPluginCount = qmldir->plugins().count(); + int qmldirPluginCount = qmldir.plugins().count(); if (qmldirPluginCount == 0) return true; @@ -1090,7 +1090,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath, // listed plugin inside qmldir. And for this reason, mixing dynamic and static plugins inside a // single module is not recommended. - QString typeNamespace = qmldir->typeNamespace(); + QString typeNamespace = qmldir.typeNamespace(); QString qmldirPath = qmldirFilePath; int slash = qmldirPath.lastIndexOf(Slash); if (slash > 0) @@ -1100,7 +1100,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath, int staticPluginsFound = 0; #if defined(QT_SHARED) - const auto qmldirPlugins = qmldir->plugins(); + const auto qmldirPlugins = qmldir.plugins(); for (const QQmlDirParser::Plugin &plugin : qmldirPlugins) { QString resolvedFilePath = database->resolvePlugin(typeLoader, qmldirPath, plugin.path, plugin.name); if (!resolvedFilePath.isEmpty()) { @@ -1166,7 +1166,7 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath, if (qmldirPluginCount > 1 && staticPluginsFound > 0) error.setDescription(QQmlImportDatabase::tr("could not resolve all plugins for module \"%1\"").arg(uri)); else - error.setDescription(QQmlImportDatabase::tr("module \"%1\" plugin \"%2\" not found").arg(uri).arg(qmldir->plugins()[dynamicPluginsFound].name)); + error.setDescription(QQmlImportDatabase::tr("module \"%1\" plugin \"%2\" not found").arg(uri).arg(qmldir.plugins()[dynamicPluginsFound].name)); error.setUrl(QUrl::fromLocalFile(qmldirFilePath)); errors->prepend(error); } @@ -1179,17 +1179,17 @@ bool QQmlImportsPrivate::importExtension(const QString &qmldirFilePath, } bool QQmlImportsPrivate::getQmldirContent(const QString &qmldirIdentifier, const QString &uri, - const QQmlTypeLoaderQmldirContent **qmldir, QList *errors) + QQmlTypeLoaderQmldirContent *qmldir, QList *errors) { Q_ASSERT(errors); Q_ASSERT(qmldir); *qmldir = typeLoader->qmldirContent(qmldirIdentifier); - if (*qmldir) { + if ((*qmldir).hasContent()) { // Ensure that parsing was successful - if ((*qmldir)->hasError()) { + if ((*qmldir).hasError()) { QUrl url = QUrl::fromLocalFile(qmldirIdentifier); - const QList qmldirErrors = (*qmldir)->errors(uri); + const QList qmldirErrors = (*qmldir).errors(uri); for (int i = 0; i < qmldirErrors.size(); ++i) { QQmlError error = qmldirErrors.at(i); error.setUrl(url); @@ -1313,14 +1313,14 @@ bool QQmlImportsPrivate::locateQmldir(const QString &uri, int vmaj, int vmin, QQ return false; } -bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent *qmldir, const QString &uri, int vmaj, int vmin, +bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent &qmldir, const QString &uri, int vmaj, int vmin, QList *errors) { int lowest_min = INT_MAX; int highest_min = INT_MIN; typedef QQmlDirComponents::const_iterator ConstIterator; - const QQmlDirComponents &components = qmldir->components(); + const QQmlDirComponents &components = qmldir.components(); ConstIterator cend = components.constEnd(); for (ConstIterator cit = components.constBegin(); cit != cend; ++cit) { @@ -1344,7 +1344,7 @@ bool QQmlImportsPrivate::validateQmldirVersion(const QQmlTypeLoaderQmldirContent } typedef QList::const_iterator SConstIterator; - const QQmlDirScripts &scripts = qmldir->scripts(); + const QQmlDirScripts &scripts = qmldir.scripts(); SConstIterator send = scripts.constEnd(); for (SConstIterator sit = scripts.constBegin(); sit != send; ++sit) { @@ -1436,14 +1436,14 @@ bool QQmlImportsPrivate::addLibraryImport(const QString& uri, const QString &pre Q_ASSERT(inserted); if (!incomplete) { - const QQmlTypeLoaderQmldirContent *qmldir = 0; + QQmlTypeLoaderQmldirContent qmldir; if (!qmldirIdentifier.isEmpty()) { if (!getQmldirContent(qmldirIdentifier, uri, &qmldir, errors)) return false; - if (qmldir) { - if (!importExtension(qmldir->pluginLocation(), uri, vmaj, vmin, database, qmldir, errors)) + if (qmldir.hasContent()) { + if (!importExtension(qmldir.pluginLocation(), uri, vmaj, vmin, database, qmldir, errors)) return false; if (!inserted->setQmldirContent(qmldirUrl, qmldir, nameSpace, errors)) @@ -1461,7 +1461,7 @@ bool QQmlImportsPrivate::addLibraryImport(const QString& uri, const QString &pre error.setDescription(QQmlImportDatabase::tr("module \"%1\" is not installed").arg(uri)); errors->prepend(error); return false; - } else if ((vmaj >= 0) && (vmin >= 0) && qmldir) { + } else if ((vmaj >= 0) && (vmin >= 0) && qmldir.hasContent()) { // Verify that the qmldir content is valid for this version if (!validateQmldirVersion(qmldir, uri, vmaj, vmin, errors)) return false; @@ -1541,12 +1541,12 @@ bool QQmlImportsPrivate::addFileImport(const QString& uri, const QString &prefix Q_ASSERT(inserted); if (!incomplete && !qmldirIdentifier.isEmpty()) { - const QQmlTypeLoaderQmldirContent *qmldir = 0; + QQmlTypeLoaderQmldirContent qmldir; if (!getQmldirContent(qmldirIdentifier, importUri, &qmldir, errors)) return false; - if (qmldir) { - if (!importExtension(qmldir->pluginLocation(), importUri, vmaj, vmin, database, qmldir, errors)) + if (qmldir.hasContent()) { + if (!importExtension(qmldir.pluginLocation(), importUri, vmaj, vmin, database, qmldir, errors)) return false; if (!inserted->setQmldirContent(url, qmldir, nameSpace, errors)) @@ -1565,14 +1565,14 @@ bool QQmlImportsPrivate::updateQmldirContent(const QString &uri, const QString & Q_ASSERT(nameSpace); if (QQmlImportInstance *import = nameSpace->findImport(uri)) { - const QQmlTypeLoaderQmldirContent *qmldir = 0; + QQmlTypeLoaderQmldirContent qmldir; if (!getQmldirContent(qmldirIdentifier, uri, &qmldir, errors)) return false; - if (qmldir) { + if (qmldir.hasContent()) { int vmaj = import->majversion; int vmin = import->minversion; - if (!importExtension(qmldir->pluginLocation(), uri, vmaj, vmin, database, qmldir, errors)) + if (!importExtension(qmldir.pluginLocation(), uri, vmaj, vmin, database, qmldir, errors)) return false; if (import->setQmldirContent(qmldirUrl, qmldir, nameSpace, errors)) { diff --git a/src/qml/qml/qqmlimport_p.h b/src/qml/qml/qqmlimport_p.h index 9cb5340c68..757179bb44 100644 --- a/src/qml/qml/qqmlimport_p.h +++ b/src/qml/qml/qqmlimport_p.h @@ -80,7 +80,7 @@ struct QQmlImportInstance QQmlDirComponents qmlDirComponents; // a copy of the components listed in the qmldir QQmlDirScripts qmlDirScripts; // a copy of the scripts in the qmldir - bool setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent *qmldir, + bool setQmldirContent(const QString &resolvedUrl, const QQmlTypeLoaderQmldirContent &qmldir, QQmlImportNamespace *nameSpace, QList *errors); static QQmlDirScripts getVersionedScripts(const QQmlDirScripts &qmldirscripts, int vmaj, int vmin); diff --git a/src/qml/qml/qqmltypeloader.cpp b/src/qml/qml/qqmltypeloader.cpp index 5318375af7..afcb9cdbe9 100644 --- a/src/qml/qml/qqmltypeloader.cpp +++ b/src/qml/qml/qqmltypeloader.cpp @@ -1379,8 +1379,8 @@ bool QQmlTypeLoader::Blob::updateQmldir(QQmlQmldirData *data, const QV4::Compile if (!importQualifier.isEmpty()) { // Does this library contain any qualified scripts? QUrl libraryUrl(qmldirUrl); - const QQmlTypeLoaderQmldirContent *qmldir = typeLoader()->qmldirContent(qmldirIdentifier); - const auto qmldirScripts = qmldir->scripts(); + const QQmlTypeLoaderQmldirContent qmldir = typeLoader()->qmldirContent(qmldirIdentifier); + const auto qmldirScripts = qmldir.scripts(); for (const QQmlDirParser::Script &script : qmldirScripts) { QUrl scriptUrl = libraryUrl.resolved(QUrl(script.fileName)); QQmlScriptBlob *blob = typeLoader()->getScript(scriptUrl); @@ -1427,8 +1427,8 @@ bool QQmlTypeLoader::Blob::addImport(const QV4::CompiledData::Import *import, QL if (!importQualifier.isEmpty()) { // Does this library contain any qualified scripts? QUrl libraryUrl(qmldirUrl); - const QQmlTypeLoaderQmldirContent *qmldir = typeLoader()->qmldirContent(qmldirFilePath); - const auto qmldirScripts = qmldir->scripts(); + const QQmlTypeLoaderQmldirContent qmldir = typeLoader()->qmldirContent(qmldirFilePath); + const auto qmldirScripts = qmldir.scripts(); for (const QQmlDirParser::Script &script : qmldirScripts) { QUrl scriptUrl = libraryUrl.resolved(QUrl(script.fileName)); QQmlScriptBlob *blob = typeLoader()->getScript(scriptUrl); @@ -1593,6 +1593,7 @@ QString QQmlTypeLoaderQmldirContent::typeNamespace() const void QQmlTypeLoaderQmldirContent::setContent(const QString &location, const QString &content) { + m_hasContent = true; m_location = location; m_parser.parse(content); } @@ -1815,6 +1816,7 @@ QString QQmlTypeLoader::absoluteFilePath(const QString &path) int lastSlash = path.lastIndexOf(QLatin1Char('/')); QString dirPath(path.left(lastSlash)); + LockHolder holder(this); if (!m_importDirCache.contains(dirPath)) { bool exists = QDir(dirPath).exists(); QCache *entry = exists ? new QCache : 0; @@ -1878,6 +1880,7 @@ bool QQmlTypeLoader::directoryExists(const QString &path) --length; QString dirPath(path.left(length)); + LockHolder holder(this); if (!m_importDirCache.contains(dirPath)) { bool exists = QDir(dirPath).exists(); QCache *files = exists ? new QCache : 0; @@ -1896,8 +1899,10 @@ Return a QQmlTypeLoaderQmldirContent for absoluteFilePath. The QQmlTypeLoaderQm It can also be a remote path for a remote directory import, but it will have been cached by now in this case. */ -const QQmlTypeLoaderQmldirContent *QQmlTypeLoader::qmldirContent(const QString &filePathIn) +const QQmlTypeLoaderQmldirContent QQmlTypeLoader::qmldirContent(const QString &filePathIn) { + LockHolder holder(this); + QString filePath; // Try to guess if filePathIn is already a URL. This is necessarily fragile, because @@ -1911,39 +1916,39 @@ const QQmlTypeLoaderQmldirContent *QQmlTypeLoader::qmldirContent(const QString & filePath = filePathIn; } else { filePath = QQmlFile::urlToLocalFileOrQrc(url); - if (filePath.isEmpty()) // Can't load the remote here, but should be cached - return *(m_importQmlDirCache.value(filePathIn)); + if (filePath.isEmpty()) { // Can't load the remote here, but should be cached + if (auto entry = m_importQmlDirCache.value(filePathIn)) + return **entry; + else + return QQmlTypeLoaderQmldirContent(); + } } - QQmlTypeLoaderQmldirContent *qmldir; QQmlTypeLoaderQmldirContent **val = m_importQmlDirCache.value(filePath); - if (!val) { - qmldir = new QQmlTypeLoaderQmldirContent; + if (val) + return **val; + QQmlTypeLoaderQmldirContent *qmldir = new QQmlTypeLoaderQmldirContent; #define ERROR(description) { QQmlError e; e.setDescription(description); qmldir->setError(e); } #define NOT_READABLE_ERROR QString(QLatin1String("module \"$$URI$$\" definition \"%1\" not readable")) #define CASE_MISMATCH_ERROR QString(QLatin1String("cannot load module \"$$URI$$\": File name case mismatch for \"%1\"")) - QFile file(filePath); - if (!QQml_isFileCaseCorrect(filePath)) { - ERROR(CASE_MISMATCH_ERROR.arg(filePath)); - } else if (file.open(QFile::ReadOnly)) { - QByteArray data = file.readAll(); - qmldir->setContent(filePath, QString::fromUtf8(data)); - } else { - ERROR(NOT_READABLE_ERROR.arg(filePath)); - } + QFile file(filePath); + if (!QQml_isFileCaseCorrect(filePath)) { + ERROR(CASE_MISMATCH_ERROR.arg(filePath)); + } else if (file.open(QFile::ReadOnly)) { + QByteArray data = file.readAll(); + qmldir->setContent(filePath, QString::fromUtf8(data)); + } else { + ERROR(NOT_READABLE_ERROR.arg(filePath)); + } #undef ERROR #undef NOT_READABLE_ERROR #undef CASE_MISMATCH_ERROR - m_importQmlDirCache.insert(filePath, qmldir); - } else { - qmldir = *val; - } - - return qmldir; + m_importQmlDirCache.insert(filePath, qmldir); + return *qmldir; } void QQmlTypeLoader::setQmldirContent(const QString &url, const QString &content) diff --git a/src/qml/qml/qqmltypeloader_p.h b/src/qml/qml/qqmltypeloader_p.h index ab32bac7b2..71bd5a8a7f 100644 --- a/src/qml/qml/qqmltypeloader_p.h +++ b/src/qml/qml/qqmltypeloader_p.h @@ -228,12 +228,16 @@ class QQmlTypeLoaderQmldirContent { private: friend class QQmlTypeLoader; - QQmlTypeLoaderQmldirContent(); void setContent(const QString &location, const QString &content); void setError(const QQmlError &); public: + QQmlTypeLoaderQmldirContent(); + QQmlTypeLoaderQmldirContent(const QQmlTypeLoaderQmldirContent &) = default; + QQmlTypeLoaderQmldirContent &operator=(const QQmlTypeLoaderQmldirContent &) = default; + + bool hasContent() const { return m_hasContent; } bool hasError() const; QList errors(const QString &uri) const; @@ -250,6 +254,7 @@ public: private: QQmlDirParser m_parser; QString m_location; + bool m_hasContent = false; }; class Q_QML_PRIVATE_EXPORT QQmlTypeLoader @@ -304,7 +309,7 @@ public: QString absoluteFilePath(const QString &path); bool directoryExists(const QString &path); - const QQmlTypeLoaderQmldirContent *qmldirContent(const QString &filePath); + const QQmlTypeLoaderQmldirContent qmldirContent(const QString &filePath); void setQmldirContent(const QString &filePath, const QString &content); void clearCache(); -- cgit v1.2.3