From a61193235279a415becfd429ab4353f05eb9b9b5 Mon Sep 17 00:00:00 2001 From: outfoxxed Date: Sat, 4 Jul 2026 17:39:45 -0700 Subject: [PATCH] core/scriptmodel: represent members as a QJSValueList Fixes UAFs caused by access to pointers stored either directly or contained in an object inside a ScriptModel which were freed and then accessed. These accesses could be observed by not setting `values` to a binding, access before updateValuesUnique finished, or randomly before a binding had been fully updated. --- changelog/next.md | 1 + src/core/scriptmodel.cpp | 56 +++++++++++++++++++---------------- src/core/scriptmodel.hpp | 15 ++++++---- src/core/test/scriptmodel.cpp | 36 ++++++++++++++-------- 4 files changed, 66 insertions(+), 42 deletions(-) diff --git a/changelog/next.md b/changelog/next.md index 9827fa0..f18fe30 100644 --- a/changelog/next.md +++ b/changelog/next.md @@ -7,3 +7,4 @@ - Fixed session lock crashes on sleep, wake, DPMS, and unlocking. - QsWindow.updatesEnabled makes sure windows are redrawn when set to true. - Fixed potential crashes from usage of `WindowsetProjection.screens` during monitor unplug. +- Fixed crashes from accessing freed objects laundered through a `ScriptModel`. diff --git a/src/core/scriptmodel.cpp b/src/core/scriptmodel.cpp index 5407e2b..9b6eb01 100644 --- a/src/core/scriptmodel.cpp +++ b/src/core/scriptmodel.cpp @@ -4,39 +4,36 @@ #include #include +#include #include #include +#include #include #include #include #include -void ScriptModel::updateValuesUnique(const QVariantList& newValues) { +bool ScriptModel::updateValuesUnique(const QJSValueList& newValues) { + auto anyChanges = false; + this->hasActiveIterators = true; this->mValues.reserve(newValues.size()); auto iter = this->mValues.begin(); auto newIter = newValues.begin(); - // TODO: cache this - auto getCmpKey = [this](const QVariant& v) { - if (v.canConvert()) { - auto vMap = v.value(); - if (vMap.contains(this->cmpKey)) { - return vMap.value(this->cmpKey); - } - } - - return v; + auto getCmpKey = [this](const QJSValue& v) { + if (v.hasProperty(this->cmpKey)) return v.property(this->cmpKey); + else return v; }; - auto variantCmp = [&, this](const QVariant& a, const QVariant& b) { - if (!this->cmpKey.isEmpty()) return getCmpKey(a) == getCmpKey(b); - else return a == b; + auto valueCmp = [&, this](const QJSValue& a, const QJSValue& b) { + if (!this->cmpKey.isEmpty()) return getCmpKey(a).strictlyEquals(getCmpKey(b)); + return a.strictlyEquals(b); }; - auto eqPredicate = [&](const QVariant& b) { - return [&](const QVariant& a) { return variantCmp(a, b); }; + auto eqPredicate = [&](const QJSValue& b) { + return [&](const QJSValue& a) { return valueCmp(a, b); }; }; while (true) { @@ -49,6 +46,7 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { this->beginRemoveRows(QModelIndex(), startIndex, endIndex); this->mValues.erase(iter, this->mValues.end()); this->endRemoveRows(); + anyChanges = true; break; } else if (iter == this->mValues.end()) { @@ -59,9 +57,10 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { this->beginInsertRows(QModelIndex(), startIndex, endIndex); this->mValues.append(newValues.sliced(startIndex)); this->endInsertRows(); + anyChanges = true; break; - } else if (!variantCmp(*newIter, *iter)) { + } else if (!valueCmp(*newIter, *iter)) { auto oldIter = std::find_if(iter, this->mValues.end(), eqPredicate(*newIter)); if (oldIter != this->mValues.end()) { @@ -81,6 +80,7 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { this->beginRemoveRows(QModelIndex(), startIndex, index - 1); iter = this->mValues.erase(startIter, iter); this->endRemoveRows(); + anyChanges = true; } else { // Advance iters to capture a whole move sequence as a single operation if possible. auto oldStartIter = oldIter; @@ -88,7 +88,7 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { ++oldIter; ++newIter; } while (oldIter != this->mValues.end() && newIter != newValues.end() - && variantCmp(*oldIter, *newIter)); + && valueCmp(*oldIter, *newIter)); auto index = static_cast(std::distance(this->mValues.begin(), iter)); auto oldStartIndex = @@ -105,6 +105,7 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { iter = this->mValues.begin() + (index + len); this->endMoveRows(); + anyChanges = true; } } else { auto startNewIter = newIter; @@ -130,8 +131,9 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { std::move_backward(iter, this->mValues.end() - len, this->mValues.end()); iter = std::copy(startNewIter, newIter, iter); this->endInsertRows(); + anyChanges = true; } - } else if (*newIter != *iter) { + } else if (!newIter->strictlyEquals(*iter)) { auto first = static_cast(std::distance(this->mValues.begin(), iter)); auto index = first; @@ -140,13 +142,16 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { ++iter; ++newIter; ++index; - } while (iter != this->mValues.end() && newIter != newValues.end() && *newIter != *iter); + } while (iter != this->mValues.end() && newIter != newValues.end() + && !newIter->strictlyEquals(*iter)); this->dataChanged( this->index(first, 0, QModelIndex()), this->index(index - 1, 0, QModelIndex()), {Qt::UserRole} ); + + anyChanges = true; } else { ++iter; ++newIter; @@ -154,12 +159,13 @@ void ScriptModel::updateValuesUnique(const QVariantList& newValues) { } this->hasActiveIterators = false; + + return anyChanges; } -void ScriptModel::setValues(const QVariantList& newValues) { - if (newValues == this->mValues) return; - this->updateValuesUnique(newValues); - emit this->valuesChanged(); +void ScriptModel::setValues(const QJSValueList& newValues) { + auto changed = this->updateValuesUnique(newValues); + if (changed) emit this->valuesChanged(); } void ScriptModel::setObjectProp(const QString& objectProp) { @@ -176,7 +182,7 @@ qint32 ScriptModel::rowCount(const QModelIndex& parent) const { QVariant ScriptModel::data(const QModelIndex& index, qint32 role) const { if (role != Qt::UserRole) return QVariant(); - return this->mValues.at(index.row()); + return this->mValues.at(index.row()).toVariant(QJSValue::RetainJSObjects); } QHash ScriptModel::roleNames() const { return {{Qt::UserRole, "modelData"}}; } diff --git a/src/core/scriptmodel.hpp b/src/core/scriptmodel.hpp index 10916f6..9a445cb 100644 --- a/src/core/scriptmodel.hpp +++ b/src/core/scriptmodel.hpp @@ -2,9 +2,13 @@ #include #include +#include +#include +#include #include #include #include +#include ///! QML model reflecting a javascript expression /// ScriptModel is a QML [Data Model] that generates model operations based on changes @@ -66,7 +70,8 @@ class ScriptModel: public QAbstractListModel { /// > values: [...DesktopEntries.applications.values].sort(...) /// > } /// > ``` - Q_PROPERTY(QVariantList values READ values WRITE setValues NOTIFY valuesChanged); + // TODO: use qjsobject for gc/free checks? + Q_PROPERTY(QJSValueList values READ values WRITE setValues NOTIFY valuesChanged); /// The property that javascript objects passed into the model will be compared with. /// /// For example, if `objectProp` is `"myprop"` then `{ myprop: "a", other: "y" }` and @@ -77,14 +82,14 @@ class ScriptModel: public QAbstractListModel { QML_ELEMENT; public: - [[nodiscard]] QVariantList values() const { + [[nodiscard]] QJSValueList values() const { auto values = this->mValues; // If not detached, the QML engine will invalidate iterators in updateValuesUnique. if (this->hasActiveIterators) values.detach(); return values; } - void setValues(const QVariantList& newValues); + void setValues(const QJSValueList& newValues); [[nodiscard]] QString objectProp() const { return this->cmpKey; } void setObjectProp(const QString& objectProp); @@ -98,9 +103,9 @@ signals: void objectPropChanged(); private: - QVariantList mValues; + QJSValueList mValues; QString cmpKey; bool hasActiveIterators = false; - void updateValuesUnique(const QVariantList& newValues); + bool updateValuesUnique(const QJSValueList& newValues); }; diff --git a/src/core/test/scriptmodel.cpp b/src/core/test/scriptmodel.cpp index b77e5e8..6a5c5f1 100644 --- a/src/core/test/scriptmodel.cpp +++ b/src/core/test/scriptmodel.cpp @@ -4,10 +4,10 @@ #include #include #include +#include #include #include #include -#include #include #include #include @@ -45,14 +45,14 @@ QDebug& operator<<(QDebug& debug, const ModelOperation& op) { } // NOLINTNEXTLINE(misc-use-internal-linkage) -QDebug& operator<<(QDebug& debug, const QVariantList& list) { +QDebug& operator<<(QDebug& debug, const QJSValueList& list) { auto str = QString(); for (const auto& var: list) { - if (var.canConvert()) { - str += var.value(); + if (var.isString()) { + str += var.toString(); } else { - qFatal() << "QVariantList debug overridden in test"; + qFatal() << "QJSValueList debug overridden in test"; } } @@ -60,6 +60,18 @@ QDebug& operator<<(QDebug& debug, const QVariantList& list) { return debug; } +namespace { +bool qjsValueListsEqual(const QJSValueList& a, const QJSValueList& b) { + if (a.length() != b.length()) return false; + + for (auto i = 0; i != a.length(); i++) { + if (!a.at(i).strictlyEquals(b.at(i))) return false; + } + + return true; +} +} // namespace + void TestScriptModel::unique_data() { QTest::addColumn("oldstr"); QTest::addColumn("newstr"); @@ -117,18 +129,18 @@ void TestScriptModel::unique() { QFETCH(const QString, newstr); QFETCH(const OpList, operations); - auto strToVariantList = [](const QString& str) -> QVariantList { - QVariantList list; + auto strToJsValueList = [](const QString& str) -> QJSValueList { + QJSValueList list; for (auto c: str) { - list.emplace_back(c); + list.emplace_back(QString(c)); } return list; }; - auto oldlist = strToVariantList(oldstr); - auto newlist = strToVariantList(newstr); + auto oldlist = strToJsValueList(oldstr); + auto newlist = strToJsValueList(newstr); auto model = ScriptModel(); auto modelTester = QAbstractItemModelTester(&model); @@ -165,7 +177,7 @@ void TestScriptModel::unique() { QObject::connect(&model, &QAbstractItemModel::rowsMoved, &model, onMove); model.setValues(oldlist); - QCOMPARE_EQ(model.values(), oldlist); + QVERIFY(qjsValueListsEqual(model.values(), oldlist)); QCOMPARE_EQ( actualOperations, OpList({{ModelOperation::Insert, 0, static_cast(oldlist.length())}}) @@ -174,7 +186,7 @@ void TestScriptModel::unique() { actualOperations.clear(); model.setValues(newlist); - QCOMPARE_EQ(model.values(), newlist); + QVERIFY(qjsValueListsEqual(model.values(), newlist)); QCOMPARE_EQ(actualOperations, operations); }