From 08505e99e004bdfe09ece062670ccae3a8ee7576 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Fri, 28 Apr 2023 12:53:24 +0200 Subject: ScriptEngine: Make import functionality exception safe Fixes: QBS-1730 Change-Id: I83324b7d859412580213dc4eb9f1f60e0f9063f2 Reviewed-by: Ivan Komissarov --- src/lib/corelib/language/evaluator.cpp | 6 +-- src/lib/corelib/language/scriptengine.cpp | 63 ++++++++++++++-------- src/lib/corelib/language/scriptengine.h | 15 +++++- src/lib/corelib/language/scriptimporter.cpp | 5 +- .../testdata/erroneous/missing-js-file.qbs | 3 ++ .../missing-js-file-module.qbs | 3 ++ .../missing-js-file-module/missing-js-file.js | 1 + tests/auto/language/tst_language.cpp | 2 + 8 files changed, 69 insertions(+), 29 deletions(-) create mode 100644 tests/auto/language/testdata/erroneous/missing-js-file.qbs create mode 100644 tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs create mode 100644 tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js diff --git a/src/lib/corelib/language/evaluator.cpp b/src/lib/corelib/language/evaluator.cpp index 5252f06be..f7c3824f2 100644 --- a/src/lib/corelib/language/evaluator.cpp +++ b/src/lib/corelib/language/evaluator.cpp @@ -269,9 +269,9 @@ Evaluator::FileContextScopes Evaluator::fileContextScopes(const FileContextConst } if (!JS_IsObject(result.importScope)) { try { - result.importScope = m_scriptEngine->newObject(); - setupScriptEngineForFile(m_scriptEngine, file, result.importScope, - ObserveMode::Enabled); + ScopedJsValue importScope(m_scriptEngine->context(), m_scriptEngine->newObject()); + setupScriptEngineForFile(m_scriptEngine, file, importScope, ObserveMode::Enabled); + result.importScope = importScope.release(); } catch (const ErrorInfo &e) { result.importScope = throwError(m_scriptEngine->context(), e.toString()); } diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp index bd7394b16..a8e6cb657 100644 --- a/src/lib/corelib/language/scriptengine.cpp +++ b/src/lib/corelib/language/scriptengine.cpp @@ -233,22 +233,7 @@ void ScriptEngine::reset() void ScriptEngine::import(const FileContextBaseConstPtr &fileCtx, JSValue &targetObject, ObserveMode observeMode) { - installImportFunctions(targetObject); - m_currentDirPathStack.push(FileInfo::path(fileCtx->filePath())); - m_extensionSearchPathsStack.push(fileCtx->searchPaths()); - m_observeMode = observeMode; - - for (const JsImport &jsImport : fileCtx->jsImports()) - import(jsImport, targetObject); - if (m_observeMode == ObserveMode::Enabled) { - for (JSValue &sv : m_requireResults) - observeImport(sv); - m_requireResults.clear(); - } - - m_currentDirPathStack.pop(); - m_extensionSearchPathsStack.pop(); - uninstallImportFunctions(); + Importer(*this, fileCtx, targetObject, observeMode).run(); } void ScriptEngine::import(const JsImport &jsImport, JSValue &targetObject) @@ -266,9 +251,10 @@ void ScriptEngine::import(const JsImport &jsImport, JSValue &targetObject) if (debugJSImports) qDebug() << "[ENGINE] " << jsImport.filePaths << " (cache miss)"; - jsImportValue = JS_NewObject(m_context); + ScopedJsValue scopedImportValue(m_context, JS_NewObject(m_context)); for (const QString &filePath : jsImport.filePaths) - importFile(filePath, jsImportValue); + importFile(filePath, scopedImportValue); + jsImportValue = scopedImportValue.release(); m_jsImportCache.insert(jsImport, jsImportValue); std::vector &filePathsForScriptValue = m_filePathsPerImport[jsObjectId(jsImportValue)]; @@ -444,7 +430,7 @@ void ScriptEngine::setEnvironment(const QProcessEnvironment &env) m_environment = env; } -void ScriptEngine::importFile(const QString &filePath, JSValue &targetObject) +void ScriptEngine::importFile(const QString &filePath, JSValue targetObject) { AccumulatingTimer importTimer(m_elapsedTimeImporting != -1 ? &m_elapsedTimeImporting : nullptr); JSValue &evaluationResult = m_jsFileCache[filePath]; @@ -598,9 +584,9 @@ JSValue ScriptEngine::js_require(JSContext *ctx, JSValueConst this_val, engine->m_logger.qbsDebug() << "[require] importing file " << filePath; } - JSValue obj = engine->newObject(); + ScopedJsValue obj(engine->context(), engine->newObject()); engine->importFile(filePath, obj); - values << obj; + values << obj.release(); filePaths.push_back(filePath); } } catch (const ErrorInfo &e) { @@ -631,8 +617,9 @@ JSValue ScriptEngine::js_require(JSContext *ctx, JSValueConst this_val, result = getJsProperty(ctx, func_data[0], scopeName); if (JS_IsObject(result)) return result; // Same JS file imported from same qbs file via different JS files (e.g. codesign.js from DarwinGCC.qbs via gcc.js and darwin.js). - result = engine->newObject(); - engine->importFile(filePath, result); + ScopedJsValue scopedResult(engine->context(), engine->newObject()); + engine->importFile(filePath, scopedResult); + result = scopedResult.release(); setJsProperty(ctx, result, StringConstants::importScopeNamePropertyInternal(), scopeName); setJsProperty(ctx, func_data[0], scopeName, result); engine->m_requireResults.push_back(result); @@ -1027,5 +1014,35 @@ void ScriptEngine::takeOwnership(JSValue v) ++m_evalResults[v]; } +ScriptEngine::Importer::Importer( + ScriptEngine &engine, const FileContextBaseConstPtr &fileCtx, JSValue &targetObject, + ObserveMode observeMode) + : m_engine(engine), m_fileCtx(fileCtx), m_targetObject(targetObject) +{ + m_engine.installImportFunctions(targetObject); + m_engine.m_currentDirPathStack.push(FileInfo::path(fileCtx->filePath())); + m_engine.m_extensionSearchPathsStack.push(fileCtx->searchPaths()); + m_engine.m_observeMode = observeMode; +} + +ScriptEngine::Importer::~Importer() +{ + if (m_engine.m_observeMode == ObserveMode::Enabled) + m_engine.m_requireResults.clear(); + m_engine.m_currentDirPathStack.pop(); + m_engine.m_extensionSearchPathsStack.pop(); + m_engine.uninstallImportFunctions(); +} + +void ScriptEngine::Importer::run() +{ + for (const JsImport &jsImport : m_fileCtx->jsImports()) + m_engine.import(jsImport, m_targetObject); + if (m_engine.m_observeMode == ObserveMode::Enabled) { + for (JSValue &sv : m_engine.m_requireResults) + m_engine.observeImport(sv); + } +} + } // namespace Internal } // namespace qbs diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h index d7798decf..8c97e3079 100644 --- a/src/lib/corelib/language/scriptengine.h +++ b/src/lib/corelib/language/scriptengine.h @@ -292,6 +292,19 @@ public: void setProperty(const char *k, const QVariant &v) { m_properties.insert(QLatin1String(k), v); } private: + class Importer { + public: + Importer(ScriptEngine &engine, const FileContextBaseConstPtr &fileCtx, + JSValue &targetObject, ObserveMode observeMode); + ~Importer(); + void run(); + + private: + ScriptEngine &m_engine; + const FileContextBaseConstPtr &m_fileCtx; + JSValue &m_targetObject; + }; + static int interruptor(JSRuntime *rt, void *opaqueEngine); bool gatherFileResults() const; @@ -305,7 +318,7 @@ private: void uninstallImportFunctions(); void import(const JsImport &jsImport, JSValue &targetObject); void observeImport(JSValue &jsImport); - void importFile(const QString &filePath, JSValue &targetObject); + void importFile(const QString &filePath, JSValue targetObject); static JSValue js_require(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic, JSValue *func_data); JSValue mergeExtensionObjects(const JSValueList &lst); diff --git a/src/lib/corelib/language/scriptimporter.cpp b/src/lib/corelib/language/scriptimporter.cpp index 1b012f3c3..fdb0689ad 100644 --- a/src/lib/corelib/language/scriptimporter.cpp +++ b/src/lib/corelib/language/scriptimporter.cpp @@ -142,10 +142,11 @@ JSValue ScriptImporter::importSourceCode(const QString &sourceCode, const QStrin code = QLatin1String("(function(){\n") + sourceCode + extractor.suffix(); } - JSValue result = m_engine->evaluate(JsValueOwner::Caller, code, filePath, 0); + ScopedJsValue result(m_engine->context(), + m_engine->evaluate(JsValueOwner::Caller, code, filePath, 0)); throwOnEvaluationError(m_engine, [&filePath] () { return CodeLocation(filePath, 0); }); copyProperties(m_engine->context(), result, targetObject); - return result; + return result.release(); } void ScriptImporter::copyProperties(JSContext *ctx, const JSValue &src, JSValue &dst) diff --git a/tests/auto/language/testdata/erroneous/missing-js-file.qbs b/tests/auto/language/testdata/erroneous/missing-js-file.qbs new file mode 100644 index 000000000..a3274a35b --- /dev/null +++ b/tests/auto/language/testdata/erroneous/missing-js-file.qbs @@ -0,0 +1,3 @@ +Product { + Depends { name: "missing-js-file-module" } +} diff --git a/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs new file mode 100644 index 000000000..31302b5c2 --- /dev/null +++ b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file-module.qbs @@ -0,0 +1,3 @@ +import "missing-js-file.js" as MissingJsFile + +Module { } diff --git a/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js new file mode 100644 index 000000000..b66048a8f --- /dev/null +++ b/tests/auto/language/testdata/erroneous/modules/missing-js-file-module/missing-js-file.js @@ -0,0 +1 @@ +var userfile = require("javascriptfile.js") diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp index 689b3bd7f..50269a7d7 100644 --- a/tests/auto/language/tst_language.cpp +++ b/tests/auto/language/tst_language.cpp @@ -947,6 +947,8 @@ void TestLanguage::erroneousFiles_data() "Product.multiplexByQbsProperties."; QTest::newRow("invalid-references") << "invalid-references.qbs:2:17.*Cannot open '.*nosuchproject.qbs'"; + QTest::newRow("missing-js-file") + << "missing-js-file-module.qbs.*Cannot open '.*javascriptfile.js'"; } void TestLanguage::erroneousFiles() -- cgit v1.2.1