From 56707fc79a9f9f5017c47a1b97a158044ba5b734 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 31 Oct 2018 11:03:25 +0000 Subject: [PATCH 1/2] JS: recognize more conditionals in useless-conditional --- javascript/ql/src/Statements/UselessConditional.ql | 9 ++++++--- .../UselessConditional/UselessConditional.expected | 3 +++ .../UselessConditional/UselessConditional.js | 13 +++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index 83a1c9a3b5b6..590739d7d427 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -120,9 +120,12 @@ predicate whitelist(Expr e) { */ predicate isConditional(ASTNode cond, Expr e) { e = cond.(IfStmt).getCondition() or + e = cond.(WhileStmt).getExpr() or + e = cond.(ForStmt).getTest() or e = cond.(ConditionalExpr).getCondition() or - e = cond.(LogAndExpr).getLeftOperand() or - e = cond.(LogOrExpr).getLeftOperand() + e = cond.(LogicalBinaryExpr).getLeftOperand() or + // Include `z` in `if (x && z)`. + isConditional(_, cond) and e = cond.(LogicalBinaryExpr).getRightOperand() } from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg @@ -132,7 +135,7 @@ where isConditional(cond, op.asExpr()) and // if `cond` is of the form ` && `, // we suggest replacing it with `, ` - if cond instanceof LogAndExpr and cv = true and not op.asExpr().isPure() then + if cond.(LogAndExpr).getLeftOperand() = op.asExpr() and cv = true and not op.asExpr().isPure() then (sel = cond and msg = "This logical 'and' expression can be replaced with a comma expression.") // otherwise we just report that `op` always evaluates to `cv` diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected index ae363adfdeab..266675e3302a 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected @@ -16,6 +16,9 @@ | UselessConditional.js:76:13:76:13 | x | This use of variable 'x' always evaluates to true. | | UselessConditional.js:82:13:82:13 | x | This use of variable 'x' always evaluates to true. | | UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. | +| UselessConditional.js:94:16:94:16 | x | This use of variable 'x' always evaluates to false. | +| UselessConditional.js:100:13:100:24 | true && true | This expression always evaluates to true. | +| UselessConditional.js:101:18:101:18 | x | This use of variable 'x' always evaluates to false. | | UselessConditionalGood.js:58:12:58:13 | x2 | This use of variable 'x2' always evaluates to false. | | UselessConditionalGood.js:69:12:69:13 | xy | This use of variable 'xy' always evaluates to false. | | UselessConditionalGood.js:85:12:85:13 | xy | This use of variable 'xy' always evaluates to false. | diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js index 179014e5c770..761a71ce73ae 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -89,4 +89,17 @@ async function awaitFlow(){ if ((x, true)); }); +(function (x, y) { + if (!x) { + while (x) { // NOT OK + f(); + } + while (true) { // OK + break; + } + if (true && true) {} // NOT OK + if (y && x) {} // NOT OK + } +}); + // semmle-extractor-options: --experimental From 87e00279741b9c10c44ff89bd0c839cde83ac0ca Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 5 Nov 2018 14:35:08 +0000 Subject: [PATCH 2/2] JS: address comments --- javascript/ql/src/Statements/UselessConditional.ql | 5 ++--- .../UselessConditional/UselessConditional.expected | 2 ++ .../Statements/UselessConditional/UselessConditional.js | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index 590739d7d427..94e44c556429 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -120,12 +120,11 @@ predicate whitelist(Expr e) { */ predicate isConditional(ASTNode cond, Expr e) { e = cond.(IfStmt).getCondition() or - e = cond.(WhileStmt).getExpr() or - e = cond.(ForStmt).getTest() or + e = cond.(LoopStmt).getTest() or e = cond.(ConditionalExpr).getCondition() or e = cond.(LogicalBinaryExpr).getLeftOperand() or // Include `z` in `if (x && z)`. - isConditional(_, cond) and e = cond.(LogicalBinaryExpr).getRightOperand() + isConditional(_, cond) and e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getRightOperand() } from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected index 266675e3302a..0790b9006b7c 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected @@ -19,6 +19,8 @@ | UselessConditional.js:94:16:94:16 | x | This use of variable 'x' always evaluates to false. | | UselessConditional.js:100:13:100:24 | true && true | This expression always evaluates to true. | | UselessConditional.js:101:18:101:18 | x | This use of variable 'x' always evaluates to false. | +| UselessConditional.js:102:19:102:19 | x | This use of variable 'x' always evaluates to false. | +| UselessConditional.js:103:23:103:23 | x | This use of variable 'x' always evaluates to false. | | UselessConditionalGood.js:58:12:58:13 | x2 | This use of variable 'x2' always evaluates to false. | | UselessConditionalGood.js:69:12:69:13 | xy | This use of variable 'xy' always evaluates to false. | | UselessConditionalGood.js:85:12:85:13 | xy | This use of variable 'xy' always evaluates to false. | diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js index 761a71ce73ae..6b5c6b63c9a3 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -99,6 +99,8 @@ async function awaitFlow(){ } if (true && true) {} // NOT OK if (y && x) {} // NOT OK + if (y && (x)) {} // NOT OK + do { } while (x); // NOT OK } });