Merge pull request #705 from asger-semmle/loop-index-concurrent-modification

Approved by mc-semmle, xiemaisi

change-notes/1.20/analysis-javascript.md

@@ -18,6 +18,7 @@
 | Incomplete regular expression for hostnames (`js/incomplete-hostname-regexp`) | correctness, security, external/cwe/cwe-020 |  Highlights hostname sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default.|
 | Incomplete URL substring sanitization | correctness, security, external/cwe/cwe-020 | Highlights URL sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results shown on LGTM by default. |
 | Incorrect suffix check (`js/incorrect-suffix-check`) | correctness, security, external/cwe/cwe-020 | Highlights error-prone suffix checks based on `indexOf`, indicating a potential violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
+| Loop iteration skipped due to shifting (`js/loop-iteration-skipped-due-to-shifting`) | correctness | Highlights code that removes an element from an array while iterating over it, causing the loop to skip over some elements. Results are shown on LGTM by default. |
 | Useless comparison test (`js/useless-comparison-test`) | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. Results are shown on LGTM by default. |
 
 ## Changes to existing queries

javascript/config/suites/javascript/correctness-more

@@ -11,6 +11,7 @@
 + semmlecode-javascript-queries/LanguageFeatures/IllegalInvocation.ql: /Correctness/Language Features
 + semmlecode-javascript-queries/LanguageFeatures/InconsistentNew.ql: /Correctness/Language Features
 + semmlecode-javascript-queries/LanguageFeatures/SpuriousArguments.ql: /Correctness/Language Features
++ semmlecode-javascript-queries/Statements/LoopIterationSkippedDueToShifting.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/MisleadingIndentationAfterControlStmt.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/ReturnOutsideFunction.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/SuspiciousUnusedLoopIterationVariable.ql: /Correctness/Statements

javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp

@@ -0,0 +1,69 @@
+<!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 subsequent 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:
+</p>
+
+<ul>
+  <li>
+    If the intention is to remove <em>every occurrence</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>
+
+</recommendation>
+<example>
+
+<p>
+In this example, a function is intended to remove "<code>..</code>" parts from a path:
+</p>
+
+<sample src="examples/LoopIterationSkippedDueToShifting.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/LoopIterationSkippedDueToShiftingGood.js" />
+
+<p>
+Alternatively, use the <code>filter</code> method:
+</p>
+
+<sample src="examples/LoopIterationSkippedDueToShiftingGoodFilter.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/LoopIterationSkippedDueToShifting.ql

@@ -0,0 +1,163 @@
+/**
+ * @name Loop iteration skipped due to shifting
+ * @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/loop-iteration-skipped-due-to-shifting
+ * @tags correctness
+ * @precision high
+ */
+import javascript
+
+/**
+ * An 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()) and
+
+    // Block flow after inspecting an array element other than that at the current index.
+    // For example, if the splice happens after inspecting `array[i + 1]`, then the next
+    // element has already been "looked at" and so it doesn't matter if we skip it.
+    not exists (IndexExpr index | cfg = index |
+      array.flowsToExpr(index.getBase()) and
+      not index.getIndex() = getIndexVariable().getAnAccess())
+  }
+
+  /**
+   * 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, "Removing an array item without adjusting the loop index '" + loop.getIndexVariable().getName() + "' causes the subsequent array item to be skipped."

javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.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/LoopIterationSkippedDueToShiftingGood.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/LoopIterationSkippedDueToShiftingGoodFilter.js

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

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

@@ -0,0 +1,3 @@
+| tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
+| tst.js:13:29:13:46 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
+| tst.js:24:9:24:26 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |

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

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

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

@@ -0,0 +1,123 @@
+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('/');
+}
+
+function inspectNextElement(string) {
+  let parts = string.split('/');
+  for (let i = 0; i < parts.length; ++i) {
+    if (i < parts.length - 1 && parts[i] === parts[i + 1]) {
+      parts.splice(i, 1); // OK - next element has been looked at
+    }
+  }
+  return parts.join('/');
+}