diff --git a/CHANGELOG b/CHANGELOG index a5093a7a24..551b1a332c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,12 @@ +v3.5.2 (XXXX-XX-XX) +------------------- + +* Fixed issue #10158: Invalid Query Crashes ArangoDB. + + This fixes traversal queries that are run on a static empty start vertex + string. + + v3.5.1 (2019-10-07) ------------------- diff --git a/arangod/Aql/KShortestPathsExecutor.cpp b/arangod/Aql/KShortestPathsExecutor.cpp index ab3f933651..1769aa61b9 100644 --- a/arangod/Aql/KShortestPathsExecutor.cpp +++ b/arangod/Aql/KShortestPathsExecutor.cpp @@ -44,6 +44,9 @@ using namespace arangodb::graph; namespace { static bool isValidId(VPackSlice id) { + if (!id.isString()) { + return false; + } TRI_ASSERT(id.isString()); arangodb::velocypack::StringRef tester(id); return tester.find('/') != std::string::npos; @@ -163,7 +166,7 @@ std::pair KShortestPathsExecutor::produceRows(OutputAql bool KShortestPathsExecutor::fetchPaths() { VPackSlice start; VPackSlice end; - do { + while (true) { // Fetch a row from upstream std::tie(_rowState, _input) = _fetcher.fetchRow(); if (!_input.isInitialized()) { @@ -178,7 +181,10 @@ bool KShortestPathsExecutor::fetchPaths() { } TRI_ASSERT(start.isString()); TRI_ASSERT(end.isString()); - } while (!_finder.startKShortestPathsTraversal(start, end)); + if (_finder.startKShortestPathsTraversal(start, end)) { + break; + } + } return true; } diff --git a/arangod/Aql/KShortestPathsNode.cpp b/arangod/Aql/KShortestPathsNode.cpp index da57184d6a..b71e790c4d 100644 --- a/arangod/Aql/KShortestPathsNode.cpp +++ b/arangod/Aql/KShortestPathsNode.cpp @@ -50,7 +50,7 @@ using namespace arangodb::aql; using namespace arangodb::graph; namespace { -static void parseNodeInput(AstNode const* node, std::string& id, Variable const*& variable) { +static void parseNodeInput(AstNode const* node, std::string& id, Variable const*& variable, char const* part) { switch (node->type) { case NODE_TYPE_REFERENCE: variable = static_cast(node->getData()); @@ -59,7 +59,7 @@ static void parseNodeInput(AstNode const* node, std::string& id, Variable const* case NODE_TYPE_VALUE: if (node->value.type != VALUE_TYPE_STRING) { THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_QUERY_PARSE, - "invalid start vertex. Must either be " + std::string("invalid ") + part + " vertex. Must either be " "an _id string or an object with _id."); } variable = nullptr; @@ -67,7 +67,7 @@ static void parseNodeInput(AstNode const* node, std::string& id, Variable const* break; default: THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_QUERY_PARSE, - "invalid start vertex. Must either be an " + std::string("invalid ") + part + " vertex. Must either be an " "_id string or an object with _id."); } } @@ -134,8 +134,8 @@ KShortestPathsNode::KShortestPathsNode(ExecutionPlan* plan, size_t id, TRI_vocba } TRI_ASSERT(_toCondition != nullptr); - parseNodeInput(start, _startVertexId, _inStartVariable); - parseNodeInput(target, _targetVertexId, _inTargetVariable); + parseNodeInput(start, _startVertexId, _inStartVariable, "start"); + parseNodeInput(target, _targetVertexId, _inTargetVariable, "target"); } /// @brief Internal constructor to clone the node. diff --git a/arangod/Aql/ShortestPathNode.cpp b/arangod/Aql/ShortestPathNode.cpp index 9518c10688..9ed4cb56f3 100644 --- a/arangod/Aql/ShortestPathNode.cpp +++ b/arangod/Aql/ShortestPathNode.cpp @@ -49,7 +49,7 @@ using namespace arangodb::aql; using namespace arangodb::graph; namespace { -static void parseNodeInput(AstNode const* node, std::string& id, Variable const*& variable) { +static void parseNodeInput(AstNode const* node, std::string& id, Variable const*& variable, char const* part) { switch (node->type) { case NODE_TYPE_REFERENCE: variable = static_cast(node->getData()); @@ -58,7 +58,7 @@ static void parseNodeInput(AstNode const* node, std::string& id, Variable const* case NODE_TYPE_VALUE: if (node->value.type != VALUE_TYPE_STRING) { THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_QUERY_PARSE, - "invalid start vertex. Must either be " + std::string("invalid ") + part + " vertex. Must either be " "an _id string or an object with _id."); } variable = nullptr; @@ -66,7 +66,7 @@ static void parseNodeInput(AstNode const* node, std::string& id, Variable const* break; default: THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_QUERY_PARSE, - "invalid start vertex. Must either be an " + std::string("invalid ") + part + " vertex. Must either be an " "_id string or an object with _id."); } } @@ -132,8 +132,8 @@ ShortestPathNode::ShortestPathNode(ExecutionPlan* plan, size_t id, TRI_vocbase_t } TRI_ASSERT(_toCondition != nullptr); - parseNodeInput(start, _startVertexId, _inStartVariable); - parseNodeInput(target, _targetVertexId, _inTargetVariable); + parseNodeInput(start, _startVertexId, _inStartVariable, "start"); + parseNodeInput(target, _targetVertexId, _inTargetVariable, "target"); } /// @brief Internal constructor to clone the node. diff --git a/arangod/Aql/TraversalExecutor.cpp b/arangod/Aql/TraversalExecutor.cpp index 799d7d60ff..9876c5e6f9 100644 --- a/arangod/Aql/TraversalExecutor.cpp +++ b/arangod/Aql/TraversalExecutor.cpp @@ -52,7 +52,7 @@ TraversalExecutorInfos::TraversalExecutorInfos( TRI_ASSERT(_traverser != nullptr); TRI_ASSERT(!_registerMapping.empty()); // _fixedSource XOR _inputRegister - TRI_ASSERT((_fixedSource.empty() && _inputRegister != ExecutionNode::MaxRegisterId) || + TRI_ASSERT(_fixedSource.empty() || (!_fixedSource.empty() && _inputRegister == ExecutionNode::MaxRegisterId)); } @@ -82,7 +82,7 @@ bool TraversalExecutorInfos::usePathOutput() const { } static std::string typeToString(TraversalExecutorInfos::OutputName type) { - switch(type) { + switch (type) { case TraversalExecutorInfos::VERTEX: return std::string{"VERTEX"}; case TraversalExecutorInfos::EDGE: @@ -122,7 +122,7 @@ RegisterId TraversalExecutorInfos::pathRegister() const { } bool TraversalExecutorInfos::usesFixedSource() const { - return !_fixedSource.empty(); + return _inputRegister == ExecutionNode::MaxRegisterId; } std::string const& TraversalExecutorInfos::getFixedSource() const { diff --git a/arangod/Graph/KShortestPathsFinder.cpp b/arangod/Graph/KShortestPathsFinder.cpp index a10643d166..c02b4d58fc 100644 --- a/arangod/Graph/KShortestPathsFinder.cpp +++ b/arangod/Graph/KShortestPathsFinder.cpp @@ -50,6 +50,8 @@ KShortestPathsFinder::~KShortestPathsFinder() {} // Sets up k-shortest-paths traversal from start to end bool KShortestPathsFinder::startKShortestPathsTraversal( arangodb::velocypack::Slice const& start, arangodb::velocypack::Slice const& end) { + TRI_ASSERT(start.isString()); + TRI_ASSERT(end.isString()); _start = arangodb::velocypack::StringRef(start); _end = arangodb::velocypack::StringRef(end); diff --git a/tests/js/server/aql/aql-graph-traverser.js b/tests/js/server/aql/aql-graph-traverser.js index 2f0abc1549..2a2e57e456 100644 --- a/tests/js/server/aql/aql-graph-traverser.js +++ b/tests/js/server/aql/aql-graph-traverser.js @@ -73,6 +73,236 @@ var createBaseGraph = function () { edge.FE = ec.save(vertex.F, vertex.E, {})._id; }; +function invalidStartVertexSuite() { + const gn = 'UnitTestGraph'; + + return { + + setUpAll: function () { + db._drop(gn + 'v1'); + db._drop(gn + 'v2'); + db._drop(gn + 'e'); + + let c; + c = db._create(gn + 'v1', { numberOfShards: 1 }); + c.insert({ _key: "test" }); + + c = db._create(gn + 'v2', { numberOfShards: 1 }); + c.insert({ _key: "test" }); + + c = db._createEdgeCollection(gn + 'e', { numberOfShards: 1 }); + c.insert({ _from: gn + "v2/test", _to: gn + "v1/test" }); + }, + + tearDownAll: function () { + db._drop(gn + 'v1'); + db._drop(gn + 'v2'); + db._drop(gn + 'e'); + }, + + testTraversalNullStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + try { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} null ${gn + 'e'} RETURN {v, e}`; + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testTraversalNumberStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} -123 ${gn + 'e'} RETURN {v, e}`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testTraversalEmptyStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} '' ${gn + 'e'} RETURN {v, e}`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testShortestPathNullStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH null TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN {v, e}`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testShortestPathNumberStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH -123 TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN {v, e}`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testShortestPathEmptyStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH '' TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN {v, e}`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testShortestPathEmptyEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH '${vn + 'v1'}/1' TO '' ${gn + 'e'} RETURN {v, e}`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testShortestPathNullEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH '${vn + 'v1'}/1' TO null ${gn + 'e'} RETURN {v, e}`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testShortestPathNumberEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH '${vn + 'v1'}/1' TO -123 ${gn + 'e'} RETURN {v, e}`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testShortestPathBothEmpty: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v, e IN ${direction} SHORTEST_PATH '' TO '' ${gn + 'e'} RETURN {v, e}`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testKShortestPathsNullStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS null TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN v`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testKShortestsPathNumberStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS -123 TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN v`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testKShortestPathsEmptyStartVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS '' TO '${vn + 'v1'}/1' ${gn + 'e'} RETURN v`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testKShortestPathsEmptyEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS '${vn + 'v1'}/1' TO '' ${gn + 'e'} RETURN v`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + }, + + testKShortestPathsNullEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS '${vn + 'v1'}/1' TO null ${gn + 'e'} RETURN v`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testKShortestPathsNumberEndVertex: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS '${vn + 'v1'}/1' TO -123 ${gn + 'e'} RETURN v`; + try { + AQL_EXECUTE(q); + fail(); + } catch (err) { + assertEqual(err.errorNum, errors.ERROR_QUERY_PARSE.code); + } + }); + }, + + testKShortestPathsBothEmpty: function () { + let directions = ["INBOUND", "OUTBOUND", "ANY"]; + directions.forEach(function(direction) { + let q = `WITH ${gn + 'v1'} ${gn + 'v2'} FOR v IN ${direction} K_SHORTEST_PATHS '' TO '' ${gn + 'e'} RETURN v`; + let res = AQL_EXECUTE(q); + assertEqual([], res.json); + assertTrue(res.warnings.length > 0); + }); + } + + }; +} + function simpleInboundOutboundSuite() { const gn = 'UnitTestGraph'; @@ -4138,6 +4368,7 @@ function pruneTraversalSuite() { return testObj; } +jsunity.run(invalidStartVertexSuite); jsunity.run(simpleInboundOutboundSuite); jsunity.run(limitSuite); jsunity.run(nestedSuite);