fix out-of-bounds attribute accessor calls (#3273)

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

arangod/Aql/AttributeAccessor.cpp

@@ -34,7 +34,8 @@ using namespace arangodb::aql;
 
 /// @brief create the accessor
 AttributeAccessor::AttributeAccessor(
-    std::vector<std::string>&& attributeParts, Variable const* variable)
+    std::vector<std::string>&& 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;

arangod/Aql/AttributeAccessor.h

@@ -43,7 +43,7 @@ struct Variable;
 /// @brief AttributeAccessor
 class AttributeAccessor {
  public:
-  AttributeAccessor(std::vector<std::string>&&, Variable const*);
+  AttributeAccessor(std::vector<std::string>&&, Variable const*, bool dataIsFromCollection);
   ~AttributeAccessor() = default;
 
   /// @brief execute the accessor

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;
 

arangod/Aql/ExecutionPlan.cpp

@@ -400,7 +400,7 @@ ExecutionNode* ExecutionPlan::createCalculation(
 
   // generate a temporary calculation node
   auto expr =
-      std::make_unique<Expression>(_ast, const_cast<AstNode*>(expression));
+      std::make_unique<Expression>(this, _ast, const_cast<AstNode*>(expression));
 
   CalculationNode* en;
   if (conditionVariable != nullptr) {

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)
+Expression::Expression(ExecutionPlan* plan, Ast* ast, AstNode* node)
-    : _ast(ast),
+    : _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::Expression(ExecutionPlan* plan, Ast* ast, arangodb::velocypack::Slice const& slice)
-    : Expression(ast, new AstNode(ast, slice.get("expression"))) {}
+    : 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<Variable const*>(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 {

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;
 

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<Expression>(ast, a);
+    auto e = std::make_unique<Expression>(en->_plan, ast, a);
 
     TRI_IF_FAILURE("IndexBlock::initialize") {
       THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG);

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<Expression>(plan->getAst(), newNode);
+              auto expr = std::make_unique<Expression>(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<Expression>(plan->getAst(), newNode);
+              auto expr = std::make_unique<Expression>(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<CalculationNode*>(info.setter)
+        new Expression(plan, ast, static_cast<CalculationNode*>(info.setter)
                                 ->expression()
                                 ->nodeForModification()
                                 ->clone(ast));

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();

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

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<LookupInfo>& 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));
 }

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);

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;