8000 fix out-of-bounds attribute accessor calls (#3273) · MohammedDeveloper/arangodb@0a71b54 · GitHub
[go: up one dir, main page]
Skip to content

Commit 0a71b54

Browse files
jsteemannfceller
jsteemann
authored and
fceller
committed
fix out-of-bounds attribute accessor calls (arangodb#3273)
1 parent f8fa46a commit 0a71b54

14 files changed

+62
-38
lines changed

‎arangod/Aql/Ast.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2687,7 +2687,7 @@ AstNode* Ast::optimizeBinaryOperatorRelational(AstNode* node) {
26872687

26882688
TRI_ASSERT(lhs->isConstant() && rhs->isConstant());
26892689

2690-
Expression exp(this, node);
2690+
Expression exp(nullptr, this, node);
26912691
FixedVarExpressionContext context;
26922692
bool mustDestroy;
26932693
// execute the expression using the C++ variant
@@ -2973,7 +2973,7 @@ AstNode* Ast::optimizeFunctionCall(AstNode* node) {
29732973
TRI_ASSERT(_query->trx() != nullptr);
29742974

29752975
if (func->hasImplementation() && node->isSimple()) {
2976-
Expression exp(this, node);
2976+
Expression exp(nullptr, this, node);
29772977
FixedVarExpressionContext context;
29782978
bool mustDestroy;
29792979
// execute the expression using the C++ variant

‎arangod/Aql/AttributeAccessor.cpp

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ using namespace arangodb::aql;
3434

3535
/// @brief create the accessor
3636
AttributeAccessor::AttributeAccessor(
37-
std::vector<std::string>&& attributeParts, Variable const* variable)
37+
std::vector<std::string>&& attributeParts, Variable const* variable,
38+
bool dataIsFromCollection)
3839
: _attributeParts(attributeParts),
3940
_variable(variable),
4041
_type(EXTRACT_MULTI) {
@@ -43,14 +44,18 @@ AttributeAccessor::AttributeAccessor(
4344
TRI_ASSERT(!_attributeParts.empty());
4445

4546
// determine accessor type
47+
// it is only safe to use the optimized accessor functions for system attributes
48+
// when the input data are collection documents. it is not safe to use them for
49+
// non-collection data, as the optimized functions may easily create out-of-bounds
50+
// accesses in that case
4651
if (_attributeParts.size() == 1) {
47-
if (attributeParts[0] == StaticStrings::KeyString) {
52+
if (dataIsFromCollection && attributeParts[0] == StaticStrings::KeyString) {
4853
_type = EXTRACT_KEY;
49-
} else if (attributeParts[0] == StaticStrings::IdString) {
54+
} else if (dataIsFromCollection && attributeParts[0] == StaticStrings::IdString) {
5055
_type = EXTRACT_ID;
51-
} else if (attributeParts[0] == StaticStrings::FromString) {
56+
} else if (dataIsFromCollection && attributeParts[0] == StaticStrings::FromString) {
5257
_type = EXTRACT_FROM;
53-
} else if (attributeParts[0] == StaticStrings::ToString) {
58+
} else if (dataIsFromCollection && attributeParts[0] == StaticStrings::ToString) {
5459
_type = EXTRACT_TO;
5560
} else {
5661
_type = EXTRACT_SINGLE;

‎arangod/Aql/AttributeAccessor.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ struct Variable;
4343
/// @brief AttributeAccessor
4444
class AttributeAccessor {
4545
public:
46-
AttributeAccessor(std::vector<std::string>&&, Variable const*);
46+
AttributeAccessor(std::vector<std::string>&&, Variable const*, bool dataIsFromCollection);
4747
~AttributeAccessor() = default;
4848

4949
/// @brief execute the accessor

‎arangod/Aql/ExecutionNode.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,7 +1332,7 @@ CalculationNode::CalculationNode(ExecutionPlan* plan,
13321332
: ExecutionNode(plan, base),
13331333
_conditionVariable(Variable::varFromVPack(plan->getAst(), base, "conditionVariable", true)),
13341334
_outVariable(Variable::varFromVPack(plan->getAst(), base, "outVariable")),
1335-
_expression(new Expression(plan->getAst(), base)),
1335+
_expression(new Expression(plan, plan->getAst(), base)),
13361336
_canRemoveIfThrows(false) {}
13371337

13381338
/// @brief toVelocyPack, for CalculationNode
@@ -1373,7 +1373,7 @@ ExecutionNode* CalculationNode::clone(ExecutionPlan* plan,
13731373
outVariable = plan->getAst()->variables()->createVariable(outVariable);
13741374
}
13751375

1376-
auto c = new CalculationNode(plan, _id, _expression->clone(plan->getAst()),
1376+
auto c = new CalculationNode(plan, _id, _expression->clone(plan, plan->getAst()),
13771377
conditionVariable, outVariable);
13781378
c->_canRemoveIfThrows = _canRemoveIfThrows;
13791379

‎arangod/Aql/ExecutionPlan.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ ExecutionNode* ExecutionPlan::createCalculation(
400400

401401
// generate a temporary calculation node
402402
auto expr =
403-
std::make_unique<Expression>(_ast, const_cast<AstNode*>(expression));
403+
std::make_unique<Expression>(this, _ast, const_cast<AstNode*>(expression));
404404

405405
CalculationNode* en;
406406
if (conditionVariable != nullptr) {

‎arangod/Aql/Expression.cpp

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
#include "Aql/AqlValue.h"
2727
#include "Aql/Ast.h"
2828
#include "Aql/AttributeAccessor.h"
29+
#include "Aql/ExecutionNode.h"
30+
#include "Aql/ExecutionPlan.h"
2931
#include "Aql/ExpressionContext.h"
3032
#include "Aql/BaseExpressionContext.h"
3133
#include "Aql/Function.h"
@@ -80,8 +82,9 @@ static void RegisterWarning(arangodb::aql::Ast const* ast,
8082
}
8183

8284
/// @brief create the expression
83-
Expression::Expression(Ast* ast, AstNode* node)
84-
: _ast(ast),
85+
Expression::Expression(ExecutionPlan* plan, Ast* ast, AstNode* node)
86+
: _plan(plan),
87+
_ast(ast),
8588
_node(node),
8689
_type(UNPROCESSED),
8790
_canThrow(true),
@@ -96,8 +99,8 @@ Expression::Expression(Ast* ast, AstNode* node)
9699
}
97100

98101
/// @brief create an expression from VPack
99-
Expression::Expression(Ast* ast, arangodb::velocypack::Slice const& slice)
100-
: Expression(ast, new AstNode(ast, slice.get("expression"))) {}
102+
Expression::Expression(ExecutionPlan* plan, Ast* ast, arangodb::velocypack::Slice const& slice)
103+
: Expression(plan, ast, new AstNode(ast, slice.get("expression"))) {}
101104

102105
/// @brief destroy the expression
103106
Expression::~Expression() {
@@ -404,8 +407,20 @@ void Expression::analyzeExpression() {
404407
if (member->type == NODE_TYPE_REFERENCE) {
405408
auto v = static_cast<Variable const*>(member->getData());
406409

410+
bool dataIsFromCollection = false;
411+
if (_plan != nullptr) {
412+
// check if the variable we are referring to is set by
413+
// a collection enumeration/index enumeration
414+
auto setter = _plan->getVarSetBy(v->id);
415+
if (setter != nullptr &&
416+
(setter->getType() == ExecutionNode::INDEX || setter->getType() == ExecutionNode::ENUMERATE_COLLECTION)) {
417+
// it is
418+
dataIsFromCollection = true;
419+
}
420+
}
421+
407422
// specialize the simple expression into an attribute accessor
408-
_accessor = new AttributeAccessor(std::move(parts), v);
423+
_accessor = new AttributeAccessor(std::move(parts), v, dataIsFromCollection);
409424
if (_accessor->isDynamic()) {
410425
_type = ATTRIBUTE_DYNAMIC;
411426
} else {

‎arangod/Aql/Expression.h

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ class AqlItemBlock;
4848
struct AqlValue;
4949
class Ast;
5050
class AttributeAccessor;
51+
class ExecutionPlan;
5152
class ExpressionContext;
5253
struct V8Expression;
5354

@@ -61,10 +62,10 @@ class Expression {
6162
Expression() = delete;
6263

6364
/// @brief constructor, using an AST start node
64-
Expression(Ast*, AstNode*);
65+
Expression(ExecutionPlan* plan, Ast*, AstNode*);
6566

6667
/// @brief constructor, using VPack
67-
Expression(Ast*, arangodb::velocypack::Slice const&);
68+
Expression(ExecutionPlan* plan, Ast*, arangodb::velocypack::Slice const&);
6869

6970
~Expression();
7071

@@ -105,10 +106,10 @@ class Expression {
105106
}
106107

107108
/// @brief clone the expression, needed to clone execution plans
108-
Expression* clone(Ast* ast) {
109+
Expression* clone(ExecutionPlan* plan, Ast* ast) {
109110
// We do not need to copy the _ast, since it is managed by the
110111
// query object and the memory management of the ASTs
111-
return new Expression(ast != nullptr ? ast : _ast, _node);
112+
return new Expression(plan, ast != nullptr ? ast : _ast, _node);
112113
}
113114

114115
/// @brief return all variables used in the expression
@@ -328,6 +329,10 @@ class Expression {
328329
bool& mustDestroy);
329330

330331
private:
332+
/// @brief the query execution plan. note: this may be a nullptr for expressions
333+
/// created in the early optimization stage!
334+
ExecutionPlan* _plan;
335+
331336
/// @brief the AST
332337
Ast* _ast;
333338

‎arangod/Aql/IndexBlock.cpp

< 1241 div class="d-flex flex-row flex-justify-end flex-1 flex-order-1 flex-sm-order-2 flex-items-center">
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ int IndexBlock::initialize() {
163163
auto instantiateExpression = [&](size_t i, size_t j, size_t k,
164164
AstNode* a) -> void {
165165
// all new AstNodes are registered with the Ast in the Query
166-
auto e = std::make_unique<Expression>(ast, a);
166+
auto e = std::make_unique<Expression>(en->_plan, ast, a);
167167

168168
TRI_IF_FAILURE("IndexBlock::initialize") {
169169
THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG);

‎arangod/Aql/OptimizerRules.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ void arangodb::aql::sortInValuesRule(Optimizer* opt,
230230

231231
auto outVar = ast->variables()->createTemporaryVariable();
232232
ExecutionNode* calculationNode = nullptr;
233-
auto expression = new Expression(ast, sorted);
233+
auto expression = new Expression(plan.get(), ast, sorted);
234234
try {
235235
calculationNode =
236236
new CalculationNode(plan.get(), plan->nextId(), expression, outVar);
@@ -1056,7 +1056,7 @@ void arangodb::aql::splitFiltersRule(Optimizer* opt,
10561056

10571057
ExecutionNode* calculationNode = nullptr;
10581058
auto outVar = plan->getAst()->variables()->createTemporaryVariable();
1059-
auto expression = new Expression(plan->getAst(), current);
1059+
auto expression = new Expression(plan.get(), plan->getAst(), current);
10601060
try {
10611061
calculationNode = new CalculationNode(plan.get(), plan->nextId(),
10621062
expression, outVar);
@@ -2038,7 +2038,7 @@ void arangodb::aql::removeFiltersCoveredByIndexRule(
20382038
} else if (newNode != condition->root()) {
20392039
// some condition is left, but it is a different one than
20402040
// the one from the FILTER node
2041-
auto expr = std::make_unique<Expression>(plan->getAst(), newNode);
2041+
auto expr = std::make_unique<Expression>(plan.get(), plan->getAst(), newNode);
20422042
CalculationNode* cn =
20432043
new CalculationNode(plan.get(), plan->nextId(), expr.get(),
20442044
calculationNode->outVariable());
@@ -3713,7 +3713,7 @@ void arangodb::aql::replaceOrWithInRule(Optimizer* opt,
37133713

37143714
if (newRoot != root) {
37153715
ExecutionNode* newNode = nullptr;
3716-
Expression* expr = new Expression(plan->getAst(), newRoot);
3716+
Expression* expr = new Expression(plan.get(), plan->getAst(), newRoot);
37173717

37183718
try {
37193719
TRI_IF_FAILURE("OptimizerRules::replaceOrWithInRuleOom") {
@@ -3902,7 +3902,7 @@ void arangodb::aql::removeRedundantOrRule(Optimizer* opt,
39023902
ExecutionNode* newNode = nullptr;
39033903
auto astNode = remover.createReplacementNode(plan->getAst());
39043904

3905-
expr = new Expression(plan->getAst(), astNode);
3905+
expr = new Expression(plan.get(), plan->getAst(), astNode);
39063906

39073907
try {
39083908
newNode =
@@ -4172,7 +4172,7 @@ void arangodb::aql::removeFiltersCoveredByTraversal(Optimizer* opt, std::unique_
41724172
} else if (newNode != condition->root()) {
41734173
// some condition is left, but it is a different one than
41744174
// the one from the FILTER node
4175-
auto expr = std::make_unique<Expression>(plan->getAst(), newNode);
4175+
auto expr = std::make_unique<Expression>(plan.get(), plan->getAst(), newNode);
41764176
CalculationNode* cn =
41774177
new CalculationNode(plan.get(), plan->nextId(), expr.get(),
41784178
calculationNode->outVariable());
@@ -4822,7 +4822,7 @@ void replaceGeoCondition(ExecutionPlan* plan, GeoIndexInfo& info) {
48224822
auto ast = plan->getAst();
48234823
CalculationNode* newNode = nullptr;
48244824
Expression* expr =
4825-
new Expression(ast, static_cast<CalculationNode*>(info.setter)
4825+
new Expression(plan, ast, static_cast<CalculationNode*>(info.setter)
48264826
->expression()
48274827
->nodeForModification()
48284828
->clone(ast));

‎arangod/Aql/TraversalConditionFinder.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ bool TraversalConditionFinder::isTrueOnNull(AstNode* node, Variable const* pathV
733733
TRI_ASSERT(_plan->getAst() != nullptr);
734734

735735
bool mustDestroy = false;
736-
Expression tmpExp(_plan->getAst(), node);
736+
Expression tmpExp(_plan, _plan->getAst(), node);
737737

738738
TRI_ASSERT(_plan->getAst()->query() != nullptr);
739739
auto trx = _plan->getAst()->query()->trx();

‎arangod/Aql/TraversalNode.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,7 @@ void TraversalNode::prepareOptions() {
550550
for (auto const& jt : _globalVertexConditions) {
551551
it.second->addMember(jt);
552552
}
553-
opts->_vertexExpressions.emplace(it.first, new Expression(ast, it.second));
553+
opts->_vertexExpressions.emplace(it.first, new Expression(_plan, ast, it.second));
554554
TRI_ASSERT(!opts->_vertexExpressions[it.first]->isV8());
555555
}
556556
if (!_globalVertexConditions.empty()) {
@@ -559,7 +559,7 @@ void TraversalNode::prepareOptions() {
559559
for (auto const& it : _globalVertexConditions) {
560560
cond->addMember(it);
561561
}
562-
opts->_baseVertexExpression = new Expression(ast, cond);
562+
opts->_baseVertexExpression = new Expression(_plan, ast, cond);
563563
TRI_ASSERT(!opts->_baseVertexExpression->isV8());
564564
}
565565
// If we use the path output the cache should activate document

‎arangod/Graph/BaseOptions.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ BaseOptions::LookupInfo::LookupInfo(arangodb::aql::Query* query,
9595

9696
read = info.get("expression");
9797
if (read.isObject()) {
98-
expression = new aql::Expression(query->ast(), read);
98+
expression = new aql::Expression(query->plan(), query->ast(), read);
9999
} else {
100100
expression = nullptr;
101101
}
@@ -117,7 +117,7 @@ BaseOptions::LookupInfo::LookupInfo(LookupInfo const& other)
117117
conditionNeedUpdate(other.conditionNeedUpdate),
118118
conditionMemberToUpdate(other.conditionMemberToUpdate) {
119119
if (other.expression != nullptr) {
120-
expression = other.expression->clone(nullptr);
120+
expression = other.expression->clone(nullptr, nullptr);
121121
}
122122
}
123123

@@ -301,7 +301,7 @@ void BaseOptions::injectLookupInfoInList(std::vector<LookupInfo>& list,
301301
condition->removeMemberUnchecked(n - 1);
302302
}
303303
}
304-
info.expression = new aql::Expression(plan->getAst(), condition);
304+
info.expression = new aql::Expression(plan, plan->getAst(), condition);
305305
}
306306
list.emplace_back(std::move(info));
307307
}

‎arangod/Graph/TraverserOptions.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -212,11 +212,11 @@ arangodb::traverser::TraverserOptions::TraverserOptions(
212212
uint64_t d = basics::StringUtils::uint64(info.key.copyString());
213213
#ifdef ARANGODB_ENABLE_MAINAINER_MODE
214214
auto it = _vertexExpressions.emplace(
215-
d, new aql::Expression(query->ast(), info.value));
215+
d, new aql::Expression(query->plan(), query->ast(), info.value));
216216
TRI_ASSERT(it.second);
217217
#else
218218
_vertexExpressions.emplace(d,
219-
new aql::Expression(query->ast(), info.value));
219+
new aql::Expression(query->plan(), query->ast(), info.value));
220220
#endif
221221
}
222222
}
@@ -228,7 +228,7 @@ arangodb::traverser::TraverserOptions::TraverserOptions(
228228
TRI_ERROR_BAD_PARAMETER,
229229
"The options require vertexExpressions to be an object");
230230
}
231-
_baseVertexExpression = new aql::Expression(query->ast(), read);
231+
_baseVertexExpression = new aql::Expression(query->plan(), query->ast(), read);
232232
}
233233
// Check for illegal option combination:
234234
TRI_ASSERT(uniqueEdges != TraverserOptions::UniquenessLevel::GLOBAL);

‎arangod/Transaction/Helpers.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,6 @@ VPackSlice transaction::helpers::extractToFromDocument(VPackSlice slice) {
241241
}
242242
// this method must only be called on edges
243243
// this means we must have at least the attributes _key, _id, _from, _to and _rev
244-
245244
uint8_t const* p = slice.begin() + slice.findDataOffset(slice.head());
246245
VPackValueLength count = 0;
247246

0 commit comments

Comments
 (0)
0