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

@@ -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`.

src/core/scriptmodel.cpp

@@ -4,39 +4,36 @@
 
 #include <qabstractitemmodel.h>
 #include <qcontainerfwd.h>
+#include <qjsvalue.h>
 #include <qlist.h>
 #include <qnamespace.h>
+#include <qobject.h>
 #include <qtmetamacros.h>
 #include <qtversionchecks.h>
 #include <qtypes.h>
 #include <qvariant.h>
 
-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<QVariantMap>()) {
-			auto vMap = v.value<QVariantMap>();
-			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<qint32>(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<qint32>(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<int, QByteArray> ScriptModel::roleNames() const { return {{Qt::UserRole, "modelData"}}; }

src/core/scriptmodel.hpp

@@ -2,9 +2,13 @@
 
 #include <qabstractitemmodel.h>
 #include <qcontainerfwd.h>
+#include <qjsvalue.h>
+#include <qobject.h>
+#include <qobjectdefs.h>
 #include <qproperty.h>
 #include <qqmlintegration.h>
 #include <qtmetamacros.h>
+#include <qtypes.h>
 
 ///! 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);
 };

src/core/test/scriptmodel.cpp

@@ -4,10 +4,10 @@
 #include <qabstractitemmodeltester.h>
 #include <qcontainerfwd.h>
 #include <qdebug.h>
+#include <qjsvalue.h>
 #include <qlist.h>
 #include <qlogging.h>
 #include <qobject.h>
-#include <qsignalspy.h>
 #include <qstring.h>
 #include <qtest.h>
 #include <qtestcase.h>
@@ -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<QChar>()) {
-			str += var.value<QChar>();
+		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<QString>("oldstr");
 	QTest::addColumn<QString>("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<qint32>(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);
 }