From 2bcd1106d170cb9e35374675a72d446ec8fee9f8 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Sun, 5 Mar 2023 19:05:45 +0100 Subject: [PATCH] Fixed ScriptObjectV8Proxy access after delete --- .../src/v8/ScriptObjectV8Proxy.cpp | 14 ++++++++++- .../src/v8/ScriptObjectV8Proxy.h | 24 +++++++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp index 99eac57866..bed092bbd5 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp @@ -33,8 +33,15 @@ Q_DECLARE_METATYPE(ScriptValue) Q_DECLARE_METATYPE(QSharedPointer) Q_DECLARE_METATYPE(QSharedPointer) + +// These values are put into internal fields of V8 objects, to signalize what kind of data is the pointer in another +// internal field pointing to. Proxy unwrapping functions recognize proxies by checking for these values in internal field 0 +// of V8 object. + // Value of internal field with index 0 when object contains ScriptObjectV8Proxy pointer in internal field 1 static const void *internalPointsToQObjectProxy = (void *)0x13370000; +// Internal field value of object pointing to ScriptObjectV8Proxy is changed to this value upon proxy's deletion +static const void *internalPointsToDeletedQObjectProxy = (void *)0x13370008; static const void *internalPointsToQVariantProxy = (void *)0x13371000; //static const void *internalPointsToSignalProxy = (void *)0x13372000; static const void *internalPointsToMethodProxy = (void *)0x13373000; @@ -211,8 +218,12 @@ ScriptObjectV8Proxy::~ScriptObjectV8Proxy() { v8::Locker locker(isolate); v8::Isolate::Scope isolateScope(isolate); v8::HandleScope handleScope(isolate); - _v8Object.Reset(); if(_object) qDebug(scriptengine) << "Deleting object proxy: " << name(); + // V8TODO: once WeakPersistent pointer is added we should check if it's valid before deleting + Q_ASSERT(!_v8Object.Get(isolate)->IsNullOrUndefined()); + // This prevents unwrap function from unwrapping proxy that was deleted + _v8Object.Get(isolate)->SetAlignedPointerInInternalField(0, const_cast(internalPointsToDeletedQObjectProxy)); + _v8Object.Reset(); if (_ownsObject) { QObject* qobject = _object; if(qobject) qobject->deleteLater(); @@ -880,6 +891,7 @@ void ScriptVariantV8Proxy::v8Set(v8::Local name, v8::Local } void ScriptVariantV8Proxy::v8GetPropertyNames(const v8::PropertyCallbackInfo& info) { + //V8TODO: Only methods from the prototype should be listed. qDebug(scriptengine) << "ScriptObjectV8Proxy::v8GetPropertyNames called"; v8::HandleScope handleScope(info.GetIsolate()); auto context = info.GetIsolate()->GetCurrentContext(); diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h index be168ffda3..3beaf1e86d 100644 --- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h +++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h @@ -115,17 +115,29 @@ private: // storage InstanceMap _signalInstances; const bool _ownsObject; QPointer _object; - // V8TODO Is this necessary? - // v8::UniquePersistent _v8ObjectTemplate; // Handle for its own object + // V8TODO Maybe depending on object ownership it should be different type of handles? For example weak persistent would allow + // script engine-owned objects to be garbage collected. This will also need adding a garbage collector callback from V8 + // to let proxy know that it is not valid anymore v8::UniquePersistent _v8Object; int pointerCorruptionTest = 12345678; Q_DISABLE_COPY(ScriptObjectV8Proxy) }; -/// [V8] (re-)implements the translation layer between ScriptValue and QVariant where a prototype is set. -/// This object depends on a ScriptObjectV8Proxy to provide the prototype's behavior +/** + * @brief [V8] (re-)implements the translation layer between ScriptValue and QVariant where a prototype is set. + * + * This object depends on a ScriptObjectV8Proxy to provide the prototype's behavior. + * ScriptVariantV8Proxy uses prototype class which provides methods which operate on QVariant. + * Typically it's used for class with larger number of methods which has a simplified JS API. + * For example it's used to provide JS scripting interface to AnimationPointer by using methods of AnimationObject. + * To use this functionality, given type has to be registered with script engine together with its prototype: + * + * engine->setDefaultPrototype(qMetaTypeId(), engine->newQObject( + new AnimationObject(), ScriptEngine::ScriptOwnership)); + * + */ class ScriptVariantV8Proxy final { public: // construction ScriptVariantV8Proxy(ScriptEngineV8* engine, const QVariant& variant, V8ScriptValue scriptProto, ScriptObjectV8Proxy* proto); @@ -134,6 +146,10 @@ public: // construction static V8ScriptValue newVariant(ScriptEngineV8* engine, const QVariant& variant, V8ScriptValue proto); static ScriptVariantV8Proxy* unwrapProxy(const V8ScriptValue& val); static ScriptVariantV8Proxy* unwrapProxy(v8::Isolate* isolate, v8::Local &value); + /** + * @brief Used to retrieve QVariant pointer contained inside script value. This is indirectly used by ScriptVariantV8Proxy + * getters and setters through scriptvalue_cast and ScriptEngineV8::castValueToVariant. + */ static QVariant* unwrapQVariantPointer(v8::Isolate* isolate, const v8::Local &value); static QVariant unwrap(const V8ScriptValue& val); inline QVariant toQVariant() const { return _variant; }