JS: add query for loop index bug

Author: asger-semmle

javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp

@@ -0,0 +1,68 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+  Items can be removed from an array using the <code>splice</code> method, but when doing so,
+  all subseequent items will be shifted to a lower index. If this is done while iterating over
+  the array, the shifting may cause the loop to skip over the element immediately after the
+  removed element.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Determine what the loop is supposed to do:
+<ul>
+  <li>
+    If the intention is to remove <em>every occurence</em> of a certain value, decrement the loop counter after removing an element, to counterbalance
+    the shift.
+  </li>
+  <li>
+    If the loop is only intended to remove <em>a single value</em> from the array, consider adding a <code>break</code> after the <code>splice</code> call.
+  </li>
+  <li>
+    If the loop is deliberately skipping over elements, consider moving the index increment into the body of the loop,
+    so it is clear that the loop is not a trivial array iteration loop.
+  </li>
+</ul>
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In this example, a function is intended to remove "<code>..</code>" parts from a path:
+</p>
+
+<sample src="examples/MisleadingIndentationAfterControlStmt.js" />
+
+<p>
+However, whenever the input contain two "<code>..</code>" parts right after one another, only the first will be removed.
+For example, the string "<code>../../secret.txt</code>" will be mapped to "<code>../secret.txt</code>". After removing
+the element at index 0, the loop counter is incremented to 1, but the second "<code>..</code>" string has now been shifted down to
+index 0 and will therefore be skipped.
+</p>
+
+<p>
+One way to avoid this is to decrement the loop counter after removing an element from the array:
+</p>
+
+<sample src="examples/MisleadingIndentationAfterControlStmtGood.js" />
+
+<p>
+Alternatively, use the <code>filter</code> method:
+</p>
+
+<sample src="examples/MisleadingIndentationAfterControlStmtGoodFilter.js" />
+
+</example>
+<references>
+
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice">Array.prototype.splice()</a>.</li>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter">Array.prototype.filter()</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql

@@ -0,0 +1,155 @@
+/**
+ * @name Missing index adjustment after concurrent modification
+ * @description Removing elements from an array while iterating over it can cause the loop to skip over some elements,
+ *              unless the loop index is decremented accordingly. 
+ * @kind problem
+ * @problem.severity warning
+ * @id js/missing-index-adjustment-after-concurrent-modification
+ * @tags correctness
+ * @precision high
+ */
+import javascript
+
+/**
+ * Operation that inserts or removes elements from an array while shifting all elements
+ * occuring after the insertion/removal point.
+ *
+ * Does not include `push` and `pop` since these never shift any elements.
+ */
+class ArrayShiftingCall extends DataFlow::MethodCallNode {
+  string name;
+
+  ArrayShiftingCall() {
+    name = getMethodName() and
+    (name = "splice" or name = "shift" or name = "unshift")
+  }
+
+  DataFlow::SourceNode getArray() {
+    result = getReceiver().getALocalSource()
+  }
+}
+
+/**
+ * A call to `splice` on an array.
+ */
+class SpliceCall extends ArrayShiftingCall {
+  SpliceCall() {
+    name = "splice"
+  }
+
+  /**
+   * Gets the index from which elements are removed and possibly new elemenst are inserted.
+   */
+  DataFlow::Node getIndex() {
+    result = getArgument(0)
+  }
+
+  /**
+   * Gets the number of removed elements.
+   */
+  int getNumRemovedElements() {
+    result = getArgument(1).asExpr().getIntValue() and
+    result >= 0
+  }
+
+  /**
+   * Gets the number of inserted elements.
+   */
+  int getNumInsertedElements() {
+    result = getNumArgument() - 2 and
+    result >= 0
+  }
+}
+
+/**
+ * A `for` loop iterating over the indices of an array, in increasing order.
+ */
+class ArrayIterationLoop extends ForStmt {
+  DataFlow::SourceNode array;
+  LocalVariable indexVariable;
+  
+  ArrayIterationLoop() {
+    exists (RelationalComparison compare | compare = getTest() |
+      compare.getLesserOperand() = indexVariable.getAnAccess() and
+      compare.getGreaterOperand() = array.getAPropertyRead("length").asExpr()) and
+    getUpdate().(IncExpr).getOperand() = indexVariable.getAnAccess()
+  }
+
+  /**
+   * Gets the variable holding the loop variable and current array index.
+   */
+  LocalVariable getIndexVariable() {
+    result = indexVariable
+  }
+
+  /**
+   * Gets the loop entry point.
+   */
+  ReachableBasicBlock getLoopEntry() {
+    result = getTest().getFirstControlFlowNode().getBasicBlock()
+  }
+
+  /**
+   * Gets a call that potentially shifts the elements of the given array.
+   */
+  ArrayShiftingCall getAnArrayShiftingCall() {
+    result.getArray() = array
+  }
+
+  /**
+   * Gets a call to `splice` that removes elements from the looped-over array at the current index 
+   *
+   * The `splice` call is not guaranteed to be inside the loop body.
+   */
+  SpliceCall getACandidateSpliceCall() {
+    result = getAnArrayShiftingCall() and
+    result.getIndex().asExpr() = getIndexVariable().getAnAccess() and
+    result.getNumRemovedElements() > result.getNumInsertedElements()
+  }
+
+  /**
+   * Holds if `cfg` modifies the index variable or shifts array elements, disturbing the
+   * relationship between the array and the index variable.
+   */
+  predicate hasIndexingManipulation(ControlFlowNode cfg) {
+    cfg.(VarDef).getAVariable() = getIndexVariable() or
+    cfg = getAnArrayShiftingCall().asExpr()
+  }
+
+  /**
+   * Holds if there is a `loop entry -> cfg` path that does not involve index manipulation or a successful index equality check.
+   */
+  predicate hasPathTo(ControlFlowNode cfg) {
+    exists(getACandidateSpliceCall()) and // restrict size of predicate
+    cfg = getLoopEntry().getFirstNode()
+    or
+    hasPathTo(cfg.getAPredecessor()) and
+    getLoopEntry().dominates(cfg.getBasicBlock()) and
+    not hasIndexingManipulation(cfg) and
+    // Ignore splice calls guarded by an index equality check.
+    // This indicates that the index of an element is the basis for removal, not its value,
+    // which means it may be okay to skip over elements.
+    not exists (ConditionGuardNode guard, EqualityTest test | cfg = guard |
+      test = guard.getTest() and
+      test.getAnOperand() = getIndexVariable().getAnAccess() and
+      guard.getOutcome() = test.getPolarity())
+  }
+
+  /**
+   * Holds if there is a `loop entry -> splice -> cfg` path that does not involve index manipulation,
+   * other than the `splice` call.
+   */
+  predicate hasPathThrough(SpliceCall splice, ControlFlowNode cfg) {
+    splice = getACandidateSpliceCall() and
+    cfg = splice.asExpr() and
+    hasPathTo(cfg.getAPredecessor())
+    or
+    hasPathThrough(splice, cfg.getAPredecessor()) and
+    getLoopEntry().dominates(cfg.getBasicBlock()) and
+    not hasIndexingManipulation(cfg)
+  }
+}
+
+from ArrayIterationLoop loop, SpliceCall splice
+where loop.hasPathThrough(splice, loop.getUpdate().getFirstControlFlowNode())
+select splice, "Missing loop index adjustment after removing array item. Some array items will be skipped due to shifting."

javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js

@@ -0,0 +1,9 @@
+function removePathTraversal(path) {
+  let parts = path.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === '..') {
+      parts.splice(i, 1);
+    }
+  }
+  return path.join('/');
+}

javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js

@@ -0,0 +1,10 @@
+function removePathTraversal(path) {
+  let parts = path.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === '..') {
+      parts.splice(i, 1);
+      --i; // adjust for array shift
+    }
+  }
+  return path.join('/');
+}

javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js

@@ -0,0 +1,3 @@
+function removePathTraversal(path) {
+  return path.split('/').filter(part => part !== '..').join('/');
+}

javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected

@@ -0,0 +1,3 @@
+| tst.js:4:27:4:44 | parts.splice(i, 1) | Missing loop index adjustment after removing array item. Some array items will be skipped due to shifting. |
+| tst.js:13:29:13:46 | parts.splice(i, 1) | Missing loop index adjustment after removing array item. Some array items will be skipped due to shifting. |
+| tst.js:24:9:24:26 | parts.splice(i, 1) | Missing loop index adjustment after removing array item. Some array items will be skipped due to shifting. |

javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref

@@ -0,0 +1 @@
+Statements/MissingIndexAdjustmentAfterConcurrentModification.ql

javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js

@@ -0,0 +1,113 @@
+function removeX(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === 'X') parts.splice(i, 1); // NOT OK
+  }
+  return parts.join('/');
+}
+
+function removeXInnerLoop(string, n) {
+  let parts = string.split('/');
+  for (let j = 0; j < n; ++j) {
+    for (let i = 0; i < parts.length; ++i) {
+      if (parts[i] === 'X') parts.splice(i, 1); // NOT OK
+    }
+  }
+  return parts.join('/');
+}
+
+function removeXOuterLoop(string, n) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    for (let j = 0; j < n; ++j) {
+      if (parts[i] === 'X') {
+        parts.splice(i, 1); // NOT OK
+        break;
+      }
+    }
+  }
+  return parts.join('/');
+}
+
+function decrementAfter(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === 'X') {
+        parts.splice(i, 1); // OK
+        --i;
+    }
+  }
+  return parts.join('/');
+}
+
+function postDecrementArgument(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === 'X') {
+        parts.splice(i--, 1); // OK
+    }
+  }
+  return parts.join('/');
+}
+
+
+function breakAfter(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === 'X') {
+        parts.splice(i, 1); // OK - only removes first occurrence
+        break;
+    }
+  }
+  return parts.join('/');
+}
+
+function insertNewElements(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (parts[i] === 'X') {
+        parts.splice(i, 1, '.'); // OK - no shifting due to insert
+    }
+  }
+  return parts.join('/');
+}
+
+function spliceAfterLoop(string) {
+  let parts = string.split('/');
+  let i = 0;
+  for (; i < parts.length; ++i) {
+    if (parts[i] === 'X') break;
+  }
+  if (parts[i] === 'X') {
+    parts.splice(i, 1); // OK - not inside loop 
+  }
+  return parts.join('/');
+}
+
+function spliceAfterLoopNested(string) {
+  let parts = string.split('/');
+  for (let j = 0; j < parts.length; ++j) {
+    let i = j;
+    for (; i < parts.length; ++i) {
+        if (parts[i] === 'X') break;
+    }
+    parts.splice(i, 1); // OK - not inside 'i' loop
+  }
+  return parts.join('/');
+}
+
+function removeAtSpecificPlace(string, k) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (i === k && parts[i] === 'X') parts.splice(i, 1); // OK - more complex logic
+  }
+  return parts.join('/');
+}
+
+function removeFirstAndLast(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (i === 0 || i === parts.length - 1) parts.splice(i, 1); // OK - out of scope of this query
+  }
+  return parts.join('/');
+}