From cd6eabb2d68321536c7fbc032a2a93990e891936 Mon Sep 17 00:00:00 2001 From: Kent Hansen Date: Thu, 16 Aug 2012 14:11:52 +0200 Subject: Use object identity to detect cycles in JS-to-C++ type conversion The documentation for v8::Object::GetIdentityHash() states that the hash value is not guaranteed to be unique (the current implementation just returns a random number). Hence, the hash value should not be used to determine whether an object has already been visited during type conversion; in the worst (and non-deterministic) case, the conversion will be "cut off" prematurely (due to identical hash values for two different objects), resulting in data loss. Instead, represent the visited objects as a set of V8 object handles. This is safe since the type conversion is always done on the stack, within a handle scope. Use v8::Object::GetIdentityHash() merely to implement the qHash() specialization needed for the set. V8 already provides an operator==() for handles, and it is documented to return true "if the objects to which they refer are identical", which is the behavior required by the set implementation. Task-number: QTBUG-21681 Change-Id: I1f2a1eee8f7c197c02c2ffeaaa1fc0274e8ab740 Reviewed-by: Michael Brasser Reviewed-by: Simon Hausmann --- src/qml/qml/v8/qv8engine.cpp | 20 +++++++++----------- src/qml/qml/v8/qv8engine_p.h | 20 ++++++++++++++------ src/qml/qml/v8/qv8jsonwrapper.cpp | 20 +++++++++----------- src/qml/qml/v8/qv8jsonwrapper_p.h | 13 +++++++------ 4 files changed, 39 insertions(+), 34 deletions(-) (limited to 'src') diff --git a/src/qml/qml/v8/qv8engine.cpp b/src/qml/qml/v8/qv8engine.cpp index 96110ed847..8233cb7108 100644 --- a/src/qml/qml/v8/qv8engine.cpp +++ b/src/qml/qml/v8/qv8engine.cpp @@ -925,18 +925,17 @@ v8::Local QV8Engine::variantListToJS(const QVariantList &lst) // of the JS Array, and elements being the JS Array's elements // converted to QVariants, recursively. QVariantList QV8Engine::variantListFromJS(v8::Handle jsArray, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { QVariantList result; - int hash = jsArray->GetIdentityHash(); - if (visitedObjects.contains(hash)) + if (visitedObjects.contains(jsArray)) return result; // Avoid recursion. v8::HandleScope handleScope; - visitedObjects.insert(hash); + visitedObjects.insert(jsArray); uint32_t length = jsArray->Length(); for (uint32_t i = 0; i < length; ++i) result.append(variantFromJS(jsArray->Get(i), visitedObjects)); - visitedObjects.remove(hash); + visitedObjects.remove(jsArray); return result; } @@ -958,7 +957,7 @@ v8::Local QV8Engine::variantMapToJS(const QVariantMap &vmap) // of the object, and values being the values of the JS object's // properties converted to QVariants, recursively. QVariantMap QV8Engine::variantMapFromJS(v8::Handle jsObject, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { QVariantMap result; @@ -968,18 +967,17 @@ QVariantMap QV8Engine::variantMapFromJS(v8::Handle jsObject, if (length == 0) return result; - int hash = jsObject->GetIdentityHash(); - if (visitedObjects.contains(hash)) + if (visitedObjects.contains(jsObject)) return result; // Avoid recursion. - visitedObjects.insert(hash); + visitedObjects.insert(jsObject); // TODO: Only object's own property names. Include non-enumerable properties. for (uint32_t i = 0; i < length; ++i) { v8::Handle name = propertyNames->Get(i); result.insert(QJSConverter::toString(name->ToString()), variantFromJS(jsObject->Get(name), visitedObjects)); } - visitedObjects.remove(hash); + visitedObjects.remove(jsObject); return result; } @@ -1265,7 +1263,7 @@ v8::Handle QV8Engine::variantToJS(const QVariant &value) // RegExp -> QVariant(QRegExp) // [Any other object] -> QVariantMap(...) QVariant QV8Engine::variantFromJS(v8::Handle value, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { Q_ASSERT(!value.IsEmpty()); if (value->IsUndefined()) diff --git a/src/qml/qml/v8/qv8engine_p.h b/src/qml/qml/v8/qv8engine_p.h index d885c777f7..9abdb842f9 100644 --- a/src/qml/qml/v8/qv8engine_p.h +++ b/src/qml/qml/v8/qv8engine_p.h @@ -237,6 +237,7 @@ public: class Q_QML_PRIVATE_EXPORT QV8Engine { + typedef QSet > V8ObjectSet; public: static QV8Engine* get(QJSEngine* q) { Q_ASSERT(q); return q->handle(); } static QJSEngine* get(QV8Engine* d) { Q_ASSERT(d); return d->q; } @@ -372,15 +373,15 @@ public: v8::Local variantListToJS(const QVariantList &lst); inline QVariantList variantListFromJS(v8::Handle jsArray) - { QSet visitedObjects; return variantListFromJS(jsArray, visitedObjects); } + { V8ObjectSet visitedObjects; return variantListFromJS(jsArray, visitedObjects); } v8::Local variantMapToJS(const QVariantMap &vmap); inline QVariantMap variantMapFromJS(v8::Handle jsObject) - { QSet visitedObjects; return variantMapFromJS(jsObject, visitedObjects); } + { V8ObjectSet visitedObjects; return variantMapFromJS(jsObject, visitedObjects); } v8::Handle variantToJS(const QVariant &value); inline QVariant variantFromJS(v8::Handle value) - { QSet visitedObjects; return variantFromJS(value, visitedObjects); } + { V8ObjectSet visitedObjects; return variantFromJS(value, visitedObjects); } v8::Handle jsonValueToJS(const QJsonValue &value); QJsonValue jsonValueFromJS(v8::Handle value); @@ -474,9 +475,9 @@ protected: double qtDateTimeToJsDate(const QDateTime &dt); private: - QVariantList variantListFromJS(v8::Handle jsArray, QSet &visitedObjects); - QVariantMap variantMapFromJS(v8::Handle jsObject, QSet &visitedObjects); - QVariant variantFromJS(v8::Handle value, QSet &visitedObjects); + QVariantList variantListFromJS(v8::Handle jsArray, V8ObjectSet &visitedObjects); + QVariantMap variantMapFromJS(v8::Handle jsObject, V8ObjectSet &visitedObjects); + QVariant variantFromJS(v8::Handle value, V8ObjectSet &visitedObjects); static v8::Persistent *findOwnerAndStrength(QObject *object, bool *shouldBeStrong); @@ -615,6 +616,13 @@ QV8Engine::Deletable *QV8Engine::extensionData(int index) const return 0; } +// Needed for V8ObjectSet +template<> +inline uint qHash >(const v8::Handle &object, uint /*seed*/) +{ + return object->GetIdentityHash(); +} + QT_END_NAMESPACE #endif // QQMLV8ENGINE_P_H diff --git a/src/qml/qml/v8/qv8jsonwrapper.cpp b/src/qml/qml/v8/qv8jsonwrapper.cpp index ff8b69f580..5ba59b2263 100644 --- a/src/qml/qml/v8/qv8jsonwrapper.cpp +++ b/src/qml/qml/v8/qv8jsonwrapper.cpp @@ -82,7 +82,7 @@ v8::Handle QV8JsonWrapper::fromJsonValue(const QJsonValue &value) } QJsonValue QV8JsonWrapper::toJsonValue(v8::Handle value, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { if (value->IsString()) return QJsonValue(QJSConverter::toString(value.As())); @@ -109,22 +109,21 @@ v8::Local QV8JsonWrapper::fromJsonObject(const QJsonObject &object) } QJsonObject QV8JsonWrapper::toJsonObject(v8::Handle value, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { QJsonObject result; if (!value->IsObject() || value->IsArray() || value->IsFunction()) return result; v8::Handle v8object(value.As()); - int hash = v8object->GetIdentityHash(); - if (visitedObjects.contains(hash)) { + if (visitedObjects.contains(v8object)) { // Avoid recursion. // For compatibility with QVariant{List,Map} conversion, we return an // empty object (and no error is thrown). return result; } - visitedObjects.insert(hash); + visitedObjects.insert(v8object); v8::Local propertyNames = m_engine->getOwnPropertyNames(v8object); uint32_t length = propertyNames->Length(); @@ -136,7 +135,7 @@ QJsonObject QV8JsonWrapper::toJsonObject(v8::Handle value, toJsonValue(propertyValue, visitedObjects)); } - visitedObjects.remove(hash); + visitedObjects.remove(v8object); return result; } @@ -151,22 +150,21 @@ v8::Local QV8JsonWrapper::fromJsonArray(const QJsonArray &array) } QJsonArray QV8JsonWrapper::toJsonArray(v8::Handle value, - QSet &visitedObjects) + V8ObjectSet &visitedObjects) { QJsonArray result; if (!value->IsArray()) return result; v8::Handle v8array(value.As()); - int hash = v8array->GetIdentityHash(); - if (visitedObjects.contains(hash)) { + if (visitedObjects.contains(v8array)) { // Avoid recursion. // For compatibility with QVariant{List,Map} conversion, we return an // empty array (and no error is thrown). return result; } - visitedObjects.insert(hash); + visitedObjects.insert(v8array); uint32_t length = v8array->Length(); for (uint32_t i = 0; i < length; ++i) { @@ -175,7 +173,7 @@ QJsonArray QV8JsonWrapper::toJsonArray(v8::Handle value, result.append(toJsonValue(element, visitedObjects)); } - visitedObjects.remove(hash); + visitedObjects.remove(v8array); return result; } diff --git a/src/qml/qml/v8/qv8jsonwrapper_p.h b/src/qml/qml/v8/qv8jsonwrapper_p.h index 887e1069f8..31a5ceba3e 100644 --- a/src/qml/qml/v8/qv8jsonwrapper_p.h +++ b/src/qml/qml/v8/qv8jsonwrapper_p.h @@ -66,6 +66,7 @@ QT_BEGIN_NAMESPACE class QV8Engine; class QV8JsonWrapper { + typedef QSet > V8ObjectSet; public: QV8JsonWrapper(); ~QV8JsonWrapper(); @@ -75,20 +76,20 @@ public: v8::Handle fromJsonValue(const QJsonValue &value); inline QJsonValue toJsonValue(v8::Handle value) - { QSet visitedObjects; return toJsonValue(value, visitedObjects); } + { V8ObjectSet visitedObjects; return toJsonValue(value, visitedObjects); } v8::Local fromJsonObject(const QJsonObject &object); inline QJsonObject toJsonObject(v8::Handle value) - { QSet visitedObjects; return toJsonObject(value, visitedObjects); } + { V8ObjectSet visitedObjects; return toJsonObject(value, visitedObjects); } v8::Local fromJsonArray(const QJsonArray &array); inline QJsonArray toJsonArray(v8::Handle value) - { QSet visitedObjects; return toJsonArray(value, visitedObjects); } + { V8ObjectSet visitedObjects; return toJsonArray(value, visitedObjects); } private: - QJsonValue toJsonValue(v8::Handle value, QSet &visitedObjects); - QJsonObject toJsonObject(v8::Handle value, QSet &visitedObjects); - QJsonArray toJsonArray(v8::Handle value, QSet &visitedObjects); + QJsonValue toJsonValue(v8::Handle value, V8ObjectSet &visitedObjects); + QJsonObject toJsonObject(v8::Handle value, V8ObjectSet &visitedObjects); + QJsonArray toJsonArray(v8::Handle value, V8ObjectSet &visitedObjects); QV8Engine *m_engine; }; -- cgit v1.2.3