From f0aaeb24555d6409fe183ae2bcc8bac7a0c95712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaakko=20Kera=CC=88nen?= Date: Mon, 23 Jan 2017 11:19:27 +0200 Subject: [PATCH] Fixed|Scripting|libcore: Fixed a memory leak when evaluating `and`/`or` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OperatorExpression was leaking the left operand’s result value. --- .../sdk/libcore/src/scriptsys/evaluator.cpp | 33 ++++++++++--------- .../src/scriptsys/operatorexpression.cpp | 6 ++-- .../sdk/libcore/src/scriptsys/process.cpp | 15 +++++---- doomsday/tests/test_script/main.cpp | 6 +++- 4 files changed, 34 insertions(+), 26 deletions(-) diff --git a/doomsday/sdk/libcore/src/scriptsys/evaluator.cpp b/doomsday/sdk/libcore/src/scriptsys/evaluator.cpp index 038302fe84..ece20938f1 100644 --- a/doomsday/sdk/libcore/src/scriptsys/evaluator.cpp +++ b/doomsday/sdk/libcore/src/scriptsys/evaluator.cpp @@ -61,7 +61,7 @@ DENG2_PIMPL(Evaluator) /// Namespace for the current expression. Record *names; - Expressions stack; + Expressions expressions; Results results; /// Returned when there is no result to give. @@ -76,7 +76,7 @@ DENG2_PIMPL(Evaluator) ~Impl() { - DENG2_ASSERT(stack.isEmpty()); + DENG2_ASSERT(expressions.isEmpty()); clearNames(); clearResults(); } @@ -99,11 +99,11 @@ DENG2_PIMPL(Evaluator) results.clear(); } - void clearStack() + void clearExpressions() { - while (!stack.empty()) + while (!expressions.empty()) { - ScopedExpression top = stack.takeLast(); + ScopedExpression top = expressions.takeLast(); clearNames(); names = top.names(); delete top.scope; @@ -112,12 +112,13 @@ DENG2_PIMPL(Evaluator) void pushResult(Value *value, Value *scope = 0 /*take*/) { - // NULLs are not pushed onto the results stack as they indicate that + // NULLs are not pushed onto the results expressions as they indicate that // no result was given. if (value) { - /*qDebug() << "Evaluator: Pushing result" << value->asText() << "in scope" - << (scope? scope->asText() : "null");*/ + /*qDebug() << "Evaluator: Pushing result" << value << value->asText() << "in scope" + << (scope? scope->asText() : "null") + << "result stack size:" << results.size();*/ results << ScopedResult(value, scope); } else @@ -137,8 +138,8 @@ DENG2_PIMPL(Evaluator) Value &evaluate(Expression const *expression) { - DENG2_ASSERT(names == NULL); - DENG2_ASSERT(stack.empty()); + DENG2_ASSERT(names == nullptr); + DENG2_ASSERT(expressions.empty()); //qDebug() << "Evaluator: Starting evaluation of" << expression; @@ -149,10 +150,10 @@ DENG2_PIMPL(Evaluator) // Clear the result stack. clearResults(); - while (!stack.empty()) + while (!expressions.empty()) { // Continue by processing the next step in the evaluation. - ScopedExpression top = stack.takeLast(); + ScopedExpression top = expressions.takeLast(); clearNames(); names = top.names(); /*qDebug() << "Evaluator: Evaluating latest scoped expression" << top.expression @@ -169,7 +170,7 @@ DENG2_PIMPL(Evaluator) DENG2_ASSERT(self().hasResult()); clearNames(); - current = NULL; + current = nullptr; return result(); } }; @@ -200,7 +201,7 @@ void Evaluator::reset() { d->current = nullptr; - d->clearStack(); + d->clearExpressions(); d->clearNames(); } @@ -245,7 +246,7 @@ Value &Evaluator::result() void Evaluator::push(Expression const *expression, Value *scope) { - d->stack.push_back(Impl::ScopedExpression(expression, scope)); + d->expressions.push_back(Impl::ScopedExpression(expression, scope)); } void Evaluator::pushResult(Value *value) @@ -258,7 +259,7 @@ Value *Evaluator::popResult(Value **evaluationScope) DENG2_ASSERT(d->results.size() > 0); Impl::ScopedResult result = d->results.takeLast(); - /*qDebug() << "Evaluator: Popping result" << result.result->asText() + /*qDebug() << "Evaluator: Popping result" << result.result << result.result->asText() << "in scope" << (result.scope? result.scope->asText() : "null");*/ if (evaluationScope) diff --git a/doomsday/sdk/libcore/src/scriptsys/operatorexpression.cpp b/doomsday/sdk/libcore/src/scriptsys/operatorexpression.cpp index 3c2c040f3f..aaffb4847b 100644 --- a/doomsday/sdk/libcore/src/scriptsys/operatorexpression.cpp +++ b/doomsday/sdk/libcore/src/scriptsys/operatorexpression.cpp @@ -210,7 +210,7 @@ Value *OperatorExpression::evaluate(Evaluator &evaluator) const { isResultTrue.push(evaluator); _rightOperand->push(evaluator); - return nullptr; + result = nullptr; } break; @@ -224,7 +224,7 @@ Value *OperatorExpression::evaluate(Evaluator &evaluator) const { isResultTrue.push(evaluator); _rightOperand->push(evaluator); - return nullptr; + result = nullptr; } break; @@ -325,7 +325,7 @@ Value *OperatorExpression::evaluate(Evaluator &evaluator) const // Delete the unnecessary values. if (result != rightValue) delete rightValue; - if (result != leftValue) delete leftValue; + if (result != leftValue) delete leftValue; return result; } diff --git a/doomsday/sdk/libcore/src/scriptsys/process.cpp b/doomsday/sdk/libcore/src/scriptsys/process.cpp index a94338cffe..57f1fbf90f 100644 --- a/doomsday/sdk/libcore/src/scriptsys/process.cpp +++ b/doomsday/sdk/libcore/src/scriptsys/process.cpp @@ -321,13 +321,20 @@ void Process::finish(Value *returnValue) if (topmost->type() == Context::FunctionCall) { // Return value to the new topmost level. + //qDebug() << "Process: Pushing function return value" << returnValue << returnValue->asText(); context().evaluator().pushResult(returnValue? returnValue : new NoneValue); } + else + { + DENG2_ASSERT(returnValue == nullptr); + } } else { DENG2_ASSERT(d->stack.back()->type() == Context::BaseProcess); + if (returnValue) delete returnValue; // Possible return value ignored. + // This was the last level. d->state = Stopped; } @@ -365,22 +372,18 @@ void Process::call(Function const &function, ArrayValue const &arguments, Value pushContext(new Context(Context::GlobalNamespace, this, function.globals())); } - // If the function is not in the global namespace, add its own parent - // namespace to the stack, too. - - // Create a new context. pushContext(new Context(Context::FunctionCall, this)); // If the scope is defined, create the "self" variable for it. if (self) { - context().names().add(new Variable("self", self /*taken*/)); + context().names().add(new Variable(QStringLiteral("self"), self /*taken*/)); } // Create local variables for the arguments in the new context. Function::ArgumentValues::const_iterator b = argValues.begin(); - Function::Arguments::const_iterator a = function.arguments().begin(); + Function::Arguments ::const_iterator a = function.arguments().begin(); for (; b != argValues.end() && a != function.arguments().end(); ++b, ++a) { // Records must only be passed as unowned references. diff --git a/doomsday/tests/test_script/main.cpp b/doomsday/tests/test_script/main.cpp index 98e0f16365..93aa883a78 100644 --- a/doomsday/tests/test_script/main.cpp +++ b/doomsday/tests/test_script/main.cpp @@ -34,6 +34,10 @@ int main(int argc, char **argv) app.initSubsystems(App::DisablePlugins); Script testScript(app.fileSystem().find("kitchen_sink.de")); +#if 0 + Script testScript("def returnValue(a): return a\n" + "returnValue(True) and returnValue(True)\n"); +#endif Process proc(testScript); LOG_MSG("Script parsing is complete! Executing..."); LOG_MSG("------------------------------------------------------------------------------"); @@ -49,5 +53,5 @@ int main(int argc, char **argv) } qDebug("Exiting main()..."); - return 0; + return 0; }