Sorted COLLECT: avoid nullptr deref when skipping and fix non-invalidated input variables (#8038)

* Added 2 regression tests

* Fixed test expected data

* Fix nullptr dereference

* Fix handling of non-invalidated input variables

* Try a less implicit fix

* Updated CHANGELOG

CHANGELOG

@@ -1,6 +1,10 @@
 devel
 -----
 
+* fixed possible segfault when using COLLECT with a LIMIT and an offset
+
+* fixed COLLECT forgetting top-level variables after 1000 rows
+
 * added sort-limit optimization in AQL; improves memory usage and execution
   time for some queries
 

arangod/Aql/CollectBlock.cpp

@@ -148,7 +148,8 @@ SortedCollectBlock::SortedCollectBlock(ExecutionEngine* engine, CollectNode cons
       _lastBlock(nullptr),
       _expressionRegister(ExecutionNode::MaxRegisterId),
       _collectRegister(ExecutionNode::MaxRegisterId),
-      _variableNames() {
+      _variableNames(),
+      _registersInherited(false) {
   for (auto const& p : en->_groupVariables) {
     // We know that planRegisters() has been run, so
     // getPlanNode()->_registerPlan is set up
@@ -257,6 +258,10 @@ std::pair<ExecutionState, arangodb::Result> SortedCollectBlock::initializeCursor
   _currentGroup.reset();
   _pos = 0;
   _lastBlock = nullptr;
+  if (_result != nullptr) {
+    auto r = _result.release();
+    returnBlock(r);
+  }
 
   return res;
 }
@@ -354,6 +359,8 @@ std::pair<ExecutionState, Result> SortedCollectBlock::getOrSkipSome(
     // group.
     size_t maxBlockSize = _groupRegisters.empty() ? 1 : atMost;
     _result.reset(requestBlock(maxBlockSize, nrOutRegs));
+    // We got a new block, we need to inherit registers for it
+    _registersInherited = false;
 
     TRI_ASSERT(nrInRegs <= _result->getNrRegs());
   }
@@ -386,12 +393,9 @@ std::pair<ExecutionState, Result> SortedCollectBlock::getOrSkipSome(
     AqlItemBlock* cur = _buffer.front();
     TRI_ASSERT(cur != nullptr);
 
-    // TODO this is dirty. if you have an idea how to improve this, please do.
-    // Can't we omit this?
-    if (_lastBlock == nullptr) {
-      // call only on the first row of the first block
-      TRI_ASSERT(_pos == 0);
+    if (!skipping && !_registersInherited) {
       inheritRegisters(cur, _result.get(), 0);
+      _registersInherited = true;
     }
 
     // if the current block changed, move the last block's infos into the
@@ -478,9 +482,9 @@ void SortedCollectBlock::emitGroup(AqlItemBlock const* cur, AqlItemBlock* res,
                                    size_t row, bool skipping) {
   TRI_ASSERT(res != nullptr);
 
-  // TODO removing this block doesn't seem to have any effect.
-  // find out if it's necessary, and why, and what it has to do with
-  // the inheritRegisters call in getOrSkipSome.
+  // Copy input registers from the first row. Note that the input variables that
+  // are still available after the block can only be constant over all input
+  // rows.
   if (row > 0 && !skipping) {
     // re-use already copied AqlValues
     TRI_ASSERT(cur != nullptr);

arangod/Aql/CollectBlock.h

@@ -135,6 +135,8 @@ class SortedCollectBlock final : public ExecutionBlock {
 
   /// @brief list of variables names for the registers
   std::vector<std::string> _variableNames;
+
+  bool _registersInherited;
 };
 
 class HashedCollectBlock final : public ExecutionBlock {

tests/js/server/aql/aql-optimizer-rule-remove-redundant-sorts.js

@@ -28,9 +28,10 @@
 /// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
 ////////////////////////////////////////////////////////////////////////////////
 
-var jsunity = require("jsunity");
-var helper = require("@arangodb/aql-helper");
-var isEqual = helper.isEqual;
+const _ = require('lodash');
+const jsunity = require("jsunity");
+const helper = require("@arangodb/aql-helper");
+const isEqual = helper.isEqual;
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief test suite
@@ -213,6 +214,57 @@ function optimizerRuleTestSuite () {
       });
     },
 
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test collect variable pass through
+///
+/// Regression test, input registers were only copied in the first block
+/// in sorted collect.
+////////////////////////////////////////////////////////////////////////////////
+
+
+    testCollectWithVariablePassThrough : function () {
+      const query = `
+        LET x = LENGTH(1..42)
+        FOR i IN 1..2000
+        LET d = {val: i}
+        SORT d.val
+        COLLECT v = d.val
+        RETURN {v, x}
+      `;
+
+      const expected = _.range(1, 2001).map(i => ({v: i, x: 42}));
+
+      const result = AQL_EXECUTE(query, {}, paramEnabled).json;
+      assertEqual(result.length, expected.length);
+      // Don't compare the whole arrays, for a better readable output in case
+      // of errors.
+      for (let i = 0; i < expected.length; i++) {
+        assertEqual(result[i], expected[i], `mismatch at index ${i}`);
+      }
+    },
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test collect with offset
+///
+/// Regression test, there was a segfault in sorted collect when at least one
+/// row was skipped.
+////////////////////////////////////////////////////////////////////////////////
+
+
+    testCollectWithOffset : function () {
+      const query = `
+        FOR i IN 1..2
+        LET d = {val: i}
+        SORT d.val
+        COLLECT v = d.val
+        LIMIT 1,1
+        RETURN v
+      `;
+
+      const result = AQL_EXECUTE(query, {}, paramEnabled).json;
+      assertEqual(result, [2]);
+    },
+
   };
 }