From 5040d3e26c73eb5ad698657da90c594fde4f1d24 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Dec 2018 15:39:09 +0000 Subject: [PATCH 01/10] JS: add query for loop index bug --- ...djustmentAfterConcurrentModification.qhelp | 68 ++++++++ ...exAdjustmentAfterConcurrentModification.ql | 155 ++++++++++++++++++ ...exAdjustmentAfterConcurrentModification.js | 9 + ...justmentAfterConcurrentModificationGood.js | 10 ++ ...ntAfterConcurrentModificationGoodFilter.js | 3 + ...stmentAfterConcurrentModification.expected | 3 + ...djustmentAfterConcurrentModification.qlref | 1 + .../tst.js | 113 +++++++++++++ 8 files changed, 362 insertions(+) create mode 100644 javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp create mode 100644 javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql create mode 100644 javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js create mode 100644 javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js create mode 100644 javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js create mode 100644 javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected create mode 100644 javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref create mode 100644 javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp new file mode 100644 index 00000000000..20bf93105d3 --- /dev/null +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp @@ -0,0 +1,68 @@ + + + +

+ Items can be removed from an array using the splice 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. +

+ + + + +

+Determine what the loop is supposed to do: +

    +
  • + If the intention is to remove every occurence of a certain value, decrement the loop counter after removing an element, to counterbalance + the shift. +
    • +
  • + If the loop is only intended to remove a single value from the array, consider adding a break after the splice call. +
    • +
  • + 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. +
    • +
+

+ + + + +

+In this example, a function is intended to remove ".." parts from a path: +

+ + + +

+However, whenever the input contain two ".." parts right after one another, only the first will be removed. +For example, the string "../../secret.txt" will be mapped to "../secret.txt". After removing +the element at index 0, the loop counter is incremented to 1, but the second ".." string has now been shifted down to +index 0 and will therefore be skipped. +

+ +

+One way to avoid this is to decrement the loop counter after removing an element from the array: +

+ + + +

+Alternatively, use the filter method: +

+ + + + + + +
  • MDN: Array.prototype.splice().
    • +
  • MDN: Array.prototype.filter().
    • + +     +     diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql new file mode 100644 index 00000000000..23f19afc127 --- /dev/null +++ b/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." diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js b/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js new file mode 100644 index 00000000000..ccff5c5d208 --- /dev/null +++ b/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('/'); +} diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js b/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js new file mode 100644 index 00000000000..3ebedc7ced9 --- /dev/null +++ b/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('/'); +} diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js b/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js new file mode 100644 index 00000000000..eba08d045d2 --- /dev/null +++ b/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js @@ -0,0 +1,3 @@ +function removePathTraversal(path) { + return path.split('/').filter(part => part !== '..').join('/'); +} diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected new file mode 100644 index 00000000000..8348040e873 --- /dev/null +++ b/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. | diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref new file mode 100644 index 00000000000..0367f1f3887 --- /dev/null +++ b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref @@ -0,0 +1 @@ +Statements/MissingIndexAdjustmentAfterConcurrentModification.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js new file mode 100644 index 00000000000..6de02e2c574 --- /dev/null +++ b/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('/'); +} From 280382e91ecd071d18eb55b05f5d670130ac5b8c Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Dec 2018 15:19:26 +0000 Subject: [PATCH 02/10] JS: whitelist if array access at another index is seen --- ...issingIndexAdjustmentAfterConcurrentModification.ql | 10 +++++++++- .../tst.js | 10 ++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql index 23f19afc127..23bc456ce6e 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql @@ -126,13 +126,21 @@ class ArrayIterationLoop extends ForStmt { 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()) + 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()) } /** diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js index 6de02e2c574..cebe8569c08 100644 --- a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js +++ b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js @@ -111,3 +111,13 @@ function removeFirstAndLast(string) { } 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('/'); +} From d595f20cb1971f3357e55881ab255646a52beb13 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Dec 2018 15:29:10 +0000 Subject: [PATCH 03/10] JS: add to correctness-more suite --- javascript/config/suites/javascript/correctness-more | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/config/suites/javascript/correctness-more b/javascript/config/suites/javascript/correctness-more index 2e57bcd7327..540f88387b6 100644 --- a/javascript/config/suites/javascript/correctness-more +++ b/javascript/config/suites/javascript/correctness-more @@ -12,6 +12,7 @@ + semmlecode-javascript-queries/LanguageFeatures/InconsistentNew.ql: /Correctness/Language Features + semmlecode-javascript-queries/LanguageFeatures/SpuriousArguments.ql: /Correctness/Language Features + semmlecode-javascript-queries/Statements/MisleadingIndentationAfterControlStmt.ql: /Correctness/Statements ++ semmlecode-javascript-queries/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/ReturnOutsideFunction.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/SuspiciousUnusedLoopIterationVariable.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/UselessConditional.ql: /Correctness/Statements From e1c25c81f657a41789e4f385030deaafd13175e7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Dec 2018 16:34:18 +0000 Subject: [PATCH 04/10] JS: add change note --- change-notes/1.20/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index 14a4104c7dd..f31fd00787e 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -13,6 +13,7 @@ | Double escaping or unescaping (`js/double-escaping`) | correctness, security, external/cwe/cwe-116 | Highlights potential double escaping or unescaping of special characters, indicating a possible violation of [CWE-116](https://cwe.mitre.org/data/definitions/116.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. | +| Missing index adjustment after concurrent modification (`js/missing-index-adjustment-after-concurrent-modification`) | 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 From f57454951bc491d3335f7f6b426cadf583dd6ccc Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Dec 2018 14:15:12 +0000 Subject: [PATCH 05/10] JS: move
      outside of

      element --- .../MissingIndexAdjustmentAfterConcurrentModification.qhelp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp index 20bf93105d3..3340b919ce2 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp @@ -15,6 +15,8 @@

      Determine what the loop is supposed to do: +

      +
      • If the intention is to remove every occurence of a certain value, decrement the loop counter after removing an element, to counterbalance @@ -28,7 +30,6 @@ Determine what the loop is supposed to do: so it is clear that the loop is not a trivial array iteration loop.
      -

      From f9d7f8ba11be7cd9fac14eefa67c4831a5408c0d Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 19 Dec 2018 10:10:56 +0000 Subject: [PATCH 06/10] JS: fix links in qhelp --- .../MissingIndexAdjustmentAfterConcurrentModification.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp index 3340b919ce2..b682eab0688 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp @@ -38,7 +38,7 @@ Determine what the loop is supposed to do: In this example, a function is intended to remove ".." parts from a path:

      - +

      However, whenever the input contain two ".." parts right after one another, only the first will be removed. @@ -51,13 +51,13 @@ index 0 and will therefore be skipped. One way to avoid this is to decrement the loop counter after removing an element from the array:

      - +

      Alternatively, use the filter method:

      - +       From 8c3b44a5256411309062ad30ad17e77e5fa5aea5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Jan 2019 11:12:52 +0000 Subject: [PATCH 07/10] JS: address comments --- .../MissingIndexAdjustmentAfterConcurrentModification.qhelp | 2 +- .../MissingIndexAdjustmentAfterConcurrentModification.ql | 4 ++-- ...ssingIndexAdjustmentAfterConcurrentModification.expected | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp index b682eab0688..6f85a9269dd 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp @@ -5,7 +5,7 @@

      Items can be removed from an array using the splice method, but when doing so, - all subseequent items will be shifted to a lower index. If this is done while iterating over + 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.

      diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql index 23bc456ce6e..18d302cb8da 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql +++ b/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql @@ -11,7 +11,7 @@ import javascript /** - * Operation that inserts or removes elements from an array while shifting all elements + * 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. @@ -160,4 +160,4 @@ class ArrayIterationLoop extends ForStmt { 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." +select splice, "Removing an array item without adjusting the loop index '" + loop.getIndexVariable().getName() + "' causes the subsequent array item to be skipped." diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected index 8348040e873..ae29da4a00c 100644 --- a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected +++ b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected @@ -1,3 +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. | +| tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent 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 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 item to be skipped. | From 9f22da455727ff298c505a5acd3ee2021c0466b9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Jan 2019 11:34:06 +0000 Subject: [PATCH 08/10] JS: rename query to "Loop iteration skipped due to shifting" --- change-notes/1.20/analysis-javascript.md | 2 +- ...cation.qhelp => LoopIterationSkippedDueToShifting.qhelp} | 6 +++--- ...Modification.ql => LoopIterationSkippedDueToShifting.ql} | 4 ++-- ...Modification.js => LoopIterationSkippedDueToShifting.js} | 0 ...tionGood.js => LoopIterationSkippedDueToShiftingGood.js} | 0 ...er.js => LoopIterationSkippedDueToShiftingGoodFilter.js} | 0 .../LoopIterationSkippedDueToShifting.expected | 3 +++ .../LoopIterationSkippedDueToShifting.qlref | 1 + .../tst.js | 0 ...ssingIndexAdjustmentAfterConcurrentModification.expected | 3 --- .../MissingIndexAdjustmentAfterConcurrentModification.qlref | 1 - 11 files changed, 10 insertions(+), 10 deletions(-) rename javascript/ql/src/Statements/{MissingIndexAdjustmentAfterConcurrentModification.qhelp => LoopIterationSkippedDueToShifting.qhelp} (89%) rename javascript/ql/src/Statements/{MissingIndexAdjustmentAfterConcurrentModification.ql => LoopIterationSkippedDueToShifting.ql} (97%) rename javascript/ql/src/Statements/examples/{MissingIndexAdjustmentAfterConcurrentModification.js => LoopIterationSkippedDueToShifting.js} (100%) rename javascript/ql/src/Statements/examples/{MissingIndexAdjustmentAfterConcurrentModificationGood.js => LoopIterationSkippedDueToShiftingGood.js} (100%) rename javascript/ql/src/Statements/examples/{MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js => LoopIterationSkippedDueToShiftingGoodFilter.js} (100%) create mode 100644 javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected create mode 100644 javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref rename javascript/ql/test/query-tests/Statements/{MissingIndexAdjustmentAfterConcurrentModification => LoopIterationSkippedDueToShifting}/tst.js (100%) delete mode 100644 javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected delete mode 100644 javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index f31fd00787e..3ca91c11bf7 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -13,7 +13,7 @@ | Double escaping or unescaping (`js/double-escaping`) | correctness, security, external/cwe/cwe-116 | Highlights potential double escaping or unescaping of special characters, indicating a possible violation of [CWE-116](https://cwe.mitre.org/data/definitions/116.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. | -| Missing index adjustment after concurrent modification (`js/missing-index-adjustment-after-concurrent-modification`) | 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. | +| 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 diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp similarity index 89% rename from javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp rename to javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp index 6f85a9269dd..de62639a213 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.qhelp +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp @@ -38,7 +38,7 @@ Determine what the loop is supposed to do: In this example, a function is intended to remove ".." parts from a path:

      - +

      However, whenever the input contain two ".." parts right after one another, only the first will be removed. @@ -51,13 +51,13 @@ index 0 and will therefore be skipped. One way to avoid this is to decrement the loop counter after removing an element from the array:

      - +

      Alternatively, use the filter method:

      - + diff --git a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql similarity index 97% rename from javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql rename to javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql index 18d302cb8da..e15462b25e9 100644 --- a/javascript/ql/src/Statements/MissingIndexAdjustmentAfterConcurrentModification.ql +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql @@ -1,10 +1,10 @@ /** - * @name Missing index adjustment after concurrent modification + * @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/missing-index-adjustment-after-concurrent-modification + * @id js/loop-iteration-skipped-due-to-shifting * @tags correctness * @precision high */ diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.js similarity index 100% rename from javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModification.js rename to javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.js diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGood.js similarity index 100% rename from javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGood.js rename to javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGood.js diff --git a/javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGoodFilter.js similarity index 100% rename from javascript/ql/src/Statements/examples/MissingIndexAdjustmentAfterConcurrentModificationGoodFilter.js rename to javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGoodFilter.js diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected new file mode 100644 index 00000000000..4b7becd8e16 --- /dev/null +++ b/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. | diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref new file mode 100644 index 00000000000..796046d180a --- /dev/null +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref @@ -0,0 +1 @@ +Statements/LoopIterationSkippedDueToShifting.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js similarity index 100% rename from javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/tst.js rename to javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected deleted file mode 100644 index ae29da4a00c..00000000000 --- a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.expected +++ /dev/null @@ -1,3 +0,0 @@ -| tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent 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 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 item to be skipped. | diff --git a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref b/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref deleted file mode 100644 index 0367f1f3887..00000000000 --- a/javascript/ql/test/query-tests/Statements/MissingIndexAdjustmentAfterConcurrentModification/MissingIndexAdjustmentAfterConcurrentModification.qlref +++ /dev/null @@ -1 +0,0 @@ -Statements/MissingIndexAdjustmentAfterConcurrentModification.ql \ No newline at end of file From bc59e65222fda49555c39498893dc905b0e1a478 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Jan 2019 11:42:47 +0000 Subject: [PATCH 09/10] JS: update suite file --- javascript/config/suites/javascript/correctness-more | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/config/suites/javascript/correctness-more b/javascript/config/suites/javascript/correctness-more index 540f88387b6..8552456ec1e 100644 --- a/javascript/config/suites/javascript/correctness-more +++ b/javascript/config/suites/javascript/correctness-more @@ -11,8 +11,8 @@ + 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/MissingIndexAdjustmentAfterConcurrentModification.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/ReturnOutsideFunction.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/SuspiciousUnusedLoopIterationVariable.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/UselessConditional.ql: /Correctness/Statements From f24313a215e19e90a74626c0ce7c1cb6d134e93a Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 3 Jan 2019 10:49:36 +0000 Subject: [PATCH 10/10] JS: address doc review --- .../ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp | 2 +- .../ql/src/Statements/LoopIterationSkippedDueToShifting.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp index de62639a213..ef24e63fa5a 100644 --- a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp @@ -19,7 +19,7 @@ Determine what the loop is supposed to do:
      • - If the intention is to remove every occurence of a certain value, decrement the loop counter after removing an element, to counterbalance + If the intention is to remove every occurrence of a certain value, decrement the loop counter after removing an element, to counterbalance the shift.
      • diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql index e15462b25e9..bbaca72234e 100644 --- a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql @@ -1,5 +1,5 @@ /** - * @name Loop iteration skipped due to shifting. + * @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