From 0a71b54e1f457b4f71617f8e01a5af6b30be9e25 Mon Sep 17 00:00:00 2001 From: Jan Date: Mon, 18 Sep 2017 14:01:06 +0200 Subject: [PATCH] fix out-of-bounds attribute accessor calls (#3273) --- arangod/Aql/Ast.cpp | 4 ++-- arangod/Aql/AttributeAccessor.cpp | 15 +++++++++----- arangod/Aql/AttributeAccessor.h | 2 +- arangod/Aql/ExecutionNode.cpp | 4 ++-- arangod/Aql/ExecutionPlan.cpp | 2 +- arangod/Aql/Expression.cpp | 25 +++++++++++++++++++----- arangod/Aql/Expression.h | 13 ++++++++---- arangod/Aql/IndexBlock.cpp | 2 +- arangod/Aql/OptimizerRules.cpp | 14 ++++++------- arangod/Aql/TraversalConditionFinder.cpp | 2 +- arangod/Aql/TraversalNode.cpp | 4 ++-- arangod/Graph/BaseOptions.cpp | 6 +++--- arangod/Graph/TraverserOptions.cpp | 6 +++--- arangod/Transaction/Helpers.cpp | 1 - 14 files changed, 62 insertions(+), 38 deletions(-) diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index bce9da2fc7..8dbc455299 100644 --- a/arangod/Aql/Ast.cpp +++ b/arangod/Aql/Ast.cpp @@ -2687,7 +2687,7 @@ AstNode* Ast::optimizeBinaryOperatorRelational(AstNode* node) { TRI_ASSERT(lhs->isConstant() && rhs->isConstant()); - Expression exp(this, node); + Expression exp(nullptr, this, node); FixedVarExpressionContext context; bool mustDestroy; // execute the expression using the C++ variant @@ -2973,7 +2973,7 @@ AstNode* Ast::optimizeFunctionCall(AstNode* node) { TRI_ASSERT(_query->trx() != nullptr); if (func->hasImplementation() && node->isSimple()) { - Expression exp(this, node); + Expression exp(nullptr, this, node); FixedVarExpressionContext context; bool mustDestroy; // execute the expression using the C++ variant diff --git a/arangod/Aql/AttributeAccessor.cpp b/arangod/Aql/AttributeAccessor.cpp index 27156ea136..af7ba1a7e6 100644 --- a/arangod/Aql/AttributeAccessor.cpp +++ b/arangod/Aql/AttributeAccessor.cpp @@ -34,7 +34,8 @@ using namespace arangodb::aql; /// @brief create the accessor AttributeAccessor::AttributeAccessor( - std::vector&& attributeParts, Variable const* variable) + std::vector&& attributeParts, Variable const* variable, + bool dataIsFromCollection) : _attributeParts(attributeParts), _variable(variable), _type(EXTRACT_MULTI) { @@ -43,14 +44,18 @@ AttributeAccessor::AttributeAccessor( TRI_ASSERT(!_attributeParts.empty()); // determine accessor type + // it is only safe to use the optimized accessor functions for system attributes + // when the input data are collection documents. it is not safe to use them for + // non-collection data, as the optimized functions may easily create out-of-bounds + // accesses in that case if (_attributeParts.size() == 1) { - if (attributeParts[0] == StaticStrings::KeyString) { + if (dataIsFromCollection && attributeParts[0] == StaticStrings::KeyString) { _type = EXTRACT_KEY; - } else if (attributeParts[0] == StaticStrings::IdString) { + } else if (dataIsFromCollection && attributeParts[0] == StaticStrings::IdString) { _type = EXTRACT_ID; - } else if (attributeParts[0] == StaticStrings::FromString) { + } else if (dataIsFromCollection && attributeParts[0] == StaticStrings::FromString) { _type = EXTRACT_FROM; - } else if (attributeParts[0] == StaticStrings::ToString) { + } else if (dataIsFromCollection && attributeParts[0] == StaticStrings::ToString) { _type = EXTRACT_TO; } else { _type = EXTRACT_SINGLE; diff --git a/arangod/Aql/AttributeAccessor.h b/arangod/Aql/AttributeAccessor.h index 4b83249062..d7356f0654 100644 --- a/arangod/Aql/AttributeAccessor.h +++ b/arangod/Aql/AttributeAccessor.h @@ -43,7 +43,7 @@ struct Variable; /// @brief AttributeAccessor class AttributeAccessor { public: - AttributeAccessor(std::vector&&, Variable const*); + AttributeAccessor(std::vector&&, Variable const*, bool dataIsFromCollection); ~AttributeAccessor() = default; /// @brief execute the accessor diff --git a/arangod/Aql/ExecutionNode.cpp b/arangod/Aql/ExecutionNode.cpp index ad0e892c82..046bc2aa1e 100644 --- a/arangod/Aql/ExecutionNode.cpp +++ b/arangod/Aql/ExecutionNode.cpp @@ -1332,7 +1332,7 @@ CalculationNode::CalculationNode(ExecutionPlan* plan, : ExecutionNode(plan, base), _conditionVariable(Variable::varFromVPack(plan->getAst(), base, "conditionVariable", true)), _outVariable(Variable::varFromVPack(plan->getAst(), base, "outVariable")), - _expression(new Expression(plan->getAst(), base)), + _expression(new Expression(plan, plan->getAst(), base)), _canRemoveIfThrows(false) {} /// @brief toVelocyPack, for CalculationNode @@ -1373,7 +1373,7 @@ ExecutionNode* CalculationNode::clone(ExecutionPlan* plan, outVariable = plan->getAst()->variables()->createVariable(outVariable); } - auto c = new CalculationNode(plan, _id, _expression->clone(plan->getAst()), + auto c = new CalculationNode(plan, _id, _expression->clone(plan, plan->getAst()), conditionVariable, outVariable); c->_canRemoveIfThrows = _canRemoveIfThrows; diff --git a/arangod/Aql/ExecutionPlan.cpp b/arangod/Aql/ExecutionPlan.cpp index 218a3d1770..d4bd5eaa76 100644 --- a/arangod/Aql/ExecutionPlan.cpp +++ b/arangod/Aql/ExecutionPlan.cpp @@ -400,7 +400,7 @@ ExecutionNode* ExecutionPlan::createCalculation( // generate a temporary calculation node auto expr = - std::make_unique(_ast, const_cast(expression)); + std::make_unique(this, _ast, const_cast(expression)); CalculationNode* en; if (conditionVariable != nullptr) { diff --git a/arangod/Aql/Expression.cpp b/arangod/Aql/Expression.cpp index 916089bb4e..437d3d3b68 100644 --- a/arangod/Aql/Expression.cpp +++ b/arangod/Aql/Expression.cpp @@ -26,6 +26,8 @@ #include "Aql/AqlValue.h" #include "Aql/Ast.h" #include "Aql/AttributeAccessor.h" +#include "Aql/ExecutionNode.h" +#include "Aql/ExecutionPlan.h" #include "Aql/ExpressionContext.h" #include "Aql/BaseExpressionContext.h" #include "Aql/Function.h" @@ -80,8 +82,9 @@ static void RegisterWarning(arangodb::aql::Ast const* ast, } /// @brief create the expression -Expression::Expression(Ast* ast, AstNode* node) - : _ast(ast), +Expression::Expression(ExecutionPlan* plan, Ast* ast, AstNode* node) + : _plan(plan), + _ast(ast), _node(node), _type(UNPROCESSED), _canThrow(true), @@ -96,8 +99,8 @@ Expression::Expression(Ast* ast, AstNode* node) } /// @brief create an expression from VPack -Expression::Expression(Ast* ast, arangodb::velocypack::Slice const& slice) - : Expression(ast, new AstNode(ast, slice.get("expression"))) {} +Expression::Expression(ExecutionPlan* plan, Ast* ast, arangodb::velocypack::Slice const& slice) + : Expression(plan, ast, new AstNode(ast, slice.get("expression"))) {} /// @brief destroy the expression Expression::~Expression() { @@ -404,8 +407,20 @@ void Expression::analyzeExpression() { if (member->type == NODE_TYPE_REFERENCE) { auto v = static_cast(member->getData()); + bool dataIsFromCollection = false; + if (_plan != nullptr) { + // check if the variable we are referring to is set by + // a collection enumeration/index enumeration + auto setter = _plan->getVarSetBy(v->id); + if (setter != nullptr && + (setter->getType() == ExecutionNode::INDEX || setter->getType() == ExecutionNode::ENUMERATE_COLLECTION)) { + // it is + dataIsFromCollection = true; + } + } + // specialize the simple expression into an attribute accessor - _accessor = new AttributeAccessor(std::move(parts), v); + _accessor = new AttributeAccessor(std::move(parts), v, dataIsFromCollection); if (_accessor->isDynamic()) { _type = ATTRIBUTE_DYNAMIC; } else { diff --git a/arangod/Aql/Expression.h b/arangod/Aql/Expression.h index 679ef5b377..dc42ee38b5 100644 --- a/arangod/Aql/Expression.h +++ b/arangod/Aql/Expression.h @@ -48,6 +48,7 @@ class AqlItemBlock; struct AqlValue; class Ast; class AttributeAccessor; +class ExecutionPlan; class ExpressionContext; struct V8Expression; @@ -61,10 +62,10 @@ class Expression { Expression() = delete; /// @brief constructor, using an AST start node - Expression(Ast*, AstNode*); + Expression(ExecutionPlan* plan, Ast*, AstNode*); /// @brief constructor, using VPack - Expression(Ast*, arangodb::velocypack::Slice const&); + Expression(ExecutionPlan* plan, Ast*, arangodb::velocypack::Slice const&); ~Expression(); @@ -105,10 +106,10 @@ class Expression { } /// @brief clone the expression, needed to clone execution plans - Expression* clone(Ast* ast) { + Expression* clone(ExecutionPlan* plan, Ast* ast) { // We do not need to copy the _ast, since it is managed by the // query object and the memory management of the ASTs - return new Expression(ast != nullptr ? ast : _ast, _node); + return new Expression(plan, ast != nullptr ? ast : _ast, _node); } /// @brief return all variables used in the expression @@ -328,6 +329,10 @@ class Expression { bool& mustDestroy); private: + /// @brief the query execution plan. note: this may be a nullptr for expressions + /// created in the early optimization stage! + ExecutionPlan* _plan; + /// @brief the AST Ast* _ast; diff --git a/arangod/Aql/IndexBlock.cpp b/arangod/Aql/IndexBlock.cpp index 1551968f4a..db2ffc8bc7 100644 --- a/arangod/Aql/IndexBlock.cpp +++ b/arangod/Aql/IndexBlock.cpp @@ -163,7 +163,7 @@ int IndexBlock::initialize() { auto instantiateExpression = [&](size_t i, size_t j, size_t k, AstNode* a) -> void { // all new AstNodes are registered with the Ast in the Query - auto e = std::make_unique(ast, a); + auto e = std::make_unique(en->_plan, ast, a); TRI_IF_FAILURE("IndexBlock::initialize") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index c6893d2771..f89623ee49 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -230,7 +230,7 @@ void arangodb::aql::sortInValuesRule(Optimizer* opt, auto outVar = ast->variables()->createTemporaryVariable(); ExecutionNode* calculationNode = nullptr; - auto expression = new Expression(ast, sorted); + auto expression = new Expression(plan.get(), ast, sorted); try { calculationNode = new CalculationNode(plan.get(), plan->nextId(), expression, outVar); @@ -1056,7 +1056,7 @@ void arangodb::aql::splitFiltersRule(Optimizer* opt, ExecutionNode* calculationNode = nullptr; auto outVar = plan->getAst()->variables()->createTemporaryVariable(); - auto expression = new Expression(plan->getAst(), current); + auto expression = new Expression(plan.get(), plan->getAst(), current); try { calculationNode = new CalculationNode(plan.get(), plan->nextId(), expression, outVar); @@ -2038,7 +2038,7 @@ void arangodb::aql::removeFiltersCoveredByIndexRule( } else if (newNode != condition->root()) { // some condition is left, but it is a different one than // the one from the FILTER node - auto expr = std::make_unique(plan->getAst(), newNode); + auto expr = std::make_unique(plan.get(), plan->getAst(), newNode); CalculationNode* cn = new CalculationNode(plan.get(), plan->nextId(), expr.get(), calculationNode->outVariable()); @@ -3713,7 +3713,7 @@ void arangodb::aql::replaceOrWithInRule(Optimizer* opt, if (newRoot != root) { ExecutionNode* newNode = nullptr; - Expression* expr = new Expression(plan->getAst(), newRoot); + Expression* expr = new Expression(plan.get(), plan->getAst(), newRoot); try { TRI_IF_FAILURE("OptimizerRules::replaceOrWithInRuleOom") { @@ -3902,7 +3902,7 @@ void arangodb::aql::removeRedundantOrRule(Optimizer* opt, ExecutionNode* newNode = nullptr; auto astNode = remover.createReplacementNode(plan->getAst()); - expr = new Expression(plan->getAst(), astNode); + expr = new Expression(plan.get(), plan->getAst(), astNode); try { newNode = @@ -4172,7 +4172,7 @@ void arangodb::aql::removeFiltersCoveredByTraversal(Optimizer* opt, std::unique_ } else if (newNode != condition->root()) { // some condition is left, but it is a different one than // the one from the FILTER node - auto expr = std::make_unique(plan->getAst(), newNode); + auto expr = std::make_unique(plan.get(), plan->getAst(), newNode); CalculationNode* cn = new CalculationNode(plan.get(), plan->nextId(), expr.get(), calculationNode->outVariable()); @@ -4822,7 +4822,7 @@ void replaceGeoCondition(ExecutionPlan* plan, GeoIndexInfo& info) { auto ast = plan->getAst(); CalculationNode* newNode = nullptr; Expression* expr = - new Expression(ast, static_cast(info.setter) + new Expression(plan, ast, static_cast(info.setter) ->expression() ->nodeForModification() ->clone(ast)); diff --git a/arangod/Aql/TraversalConditionFinder.cpp b/arangod/Aql/TraversalConditionFinder.cpp index 00e369a77c..ec6ede8708 100644 --- a/arangod/Aql/TraversalConditionFinder.cpp +++ b/arangod/Aql/TraversalConditionFinder.cpp @@ -733,7 +733,7 @@ bool TraversalConditionFinder::isTrueOnNull(AstNode* node, Variable const* pathV TRI_ASSERT(_plan->getAst() != nullptr); bool mustDestroy = false; - Expression tmpExp(_plan->getAst(), node); + Expression tmpExp(_plan, _plan->getAst(), node); TRI_ASSERT(_plan->getAst()->query() != nullptr); auto trx = _plan->getAst()->query()->trx(); diff --git a/arangod/Aql/TraversalNode.cpp b/arangod/Aql/TraversalNode.cpp index 9ac2b247fa..ad02883258 100644 --- a/arangod/Aql/TraversalNode.cpp +++ b/arangod/Aql/TraversalNode.cpp @@ -550,7 +550,7 @@ void TraversalNode::prepareOptions() { for (auto const& jt : _globalVertexConditions) { it.second->addMember(jt); } - opts->_vertexExpressions.emplace(it.first, new Expression(ast, it.second)); + opts->_vertexExpressions.emplace(it.first, new Expression(_plan, ast, it.second)); TRI_ASSERT(!opts->_vertexExpressions[it.first]->isV8()); } if (!_globalVertexConditions.empty()) { @@ -559,7 +559,7 @@ void TraversalNode::prepareOptions() { for (auto const& it : _globalVertexConditions) { cond->addMember(it); } - opts->_baseVertexExpression = new Expression(ast, cond); + opts->_baseVertexExpression = new Expression(_plan, ast, cond); TRI_ASSERT(!opts->_baseVertexExpression->isV8()); } // If we use the path output the cache should activate document diff --git a/arangod/Graph/BaseOptions.cpp b/arangod/Graph/BaseOptions.cpp index 58d54623e3..b18b763ad7 100644 --- a/arangod/Graph/BaseOptions.cpp +++ b/arangod/Graph/BaseOptions.cpp @@ -95,7 +95,7 @@ BaseOptions::LookupInfo::LookupInfo(arangodb::aql::Query* query, read = info.get("expression"); if (read.isObject()) { - expression = new aql::Expression(query->ast(), read); + expression = new aql::Expression(query->plan(), query->ast(), read); } else { expression = nullptr; } @@ -117,7 +117,7 @@ BaseOptions::LookupInfo::LookupInfo(LookupInfo const& other) conditionNeedUpdate(other.conditionNeedUpdate), conditionMemberToUpdate(other.conditionMemberToUpdate) { if (other.expression != nullptr) { - expression = other.expression->clone(nullptr); + expression = other.expression->clone(nullptr, nullptr); } } @@ -301,7 +301,7 @@ void BaseOptions::injectLookupInfoInList(std::vector& list, condition->removeMemberUnchecked(n - 1); } } - info.expression = new aql::Expression(plan->getAst(), condition); + info.expression = new aql::Expression(plan, plan->getAst(), condition); } list.emplace_back(std::move(info)); } diff --git a/arangod/Graph/TraverserOptions.cpp b/arangod/Graph/TraverserOptions.cpp index dfdbf0155d..3e45efb382 100644 --- a/arangod/Graph/TraverserOptions.cpp +++ b/arangod/Graph/TraverserOptions.cpp @@ -212,11 +212,11 @@ arangodb::traverser::TraverserOptions::TraverserOptions( uint64_t d = basics::StringUtils::uint64(info.key.copyString()); #ifdef ARANGODB_ENABLE_MAINAINER_MODE auto it = _vertexExpressions.emplace( - d, new aql::Expression(query->ast(), info.value)); + d, new aql::Expression(query->plan(), query->ast(), info.value)); TRI_ASSERT(it.second); #else _vertexExpressions.emplace(d, - new aql::Expression(query->ast(), info.value)); + new aql::Expression(query->plan(), query->ast(), info.value)); #endif } } @@ -228,7 +228,7 @@ arangodb::traverser::TraverserOptions::TraverserOptions( TRI_ERROR_BAD_PARAMETER, "The options require vertexExpressions to be an object"); } - _baseVertexExpression = new aql::Expression(query->ast(), read); + _baseVertexExpression = new aql::Expression(query->plan(), query->ast(), read); } // Check for illegal option combination: TRI_ASSERT(uniqueEdges != TraverserOptions::UniquenessLevel::GLOBAL); diff --git a/arangod/Transaction/Helpers.cpp b/arangod/Transaction/Helpers.cpp index 5010703aea..df52b6a48d 100644 --- a/arangod/Transaction/Helpers.cpp +++ b/arangod/Transaction/Helpers.cpp @@ -241,7 +241,6 @@ VPackSlice transaction::helpers::extractToFromDocument(VPackSlice slice) { } // this method must only be called on edges // this means we must have at least the attributes _key, _id, _from, _to and _rev - uint8_t const* p = slice.begin() + slice.findDataOffset(slice.head()); VPackValueLength count = 0;