From 1bf78ba633e218c2f9edea0c8e42d511817ca9f0 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Mon, 10 Dec 2018 11:47:43 +0100 Subject: [PATCH] C#: Remove `cs/non-short-circuit` query --- change-notes/1.20/analysis-csharp.md | 3 +- .../DangerousNonShortCircuitLogic.cs | 11 ---- .../DangerousNonShortCircuitLogic.qhelp | 35 ------------ .../DangerousNonShortCircuitLogic.ql | 57 ------------------- .../DangerousNonShortCircuitLogicFix.cs | 11 ---- .../DangerousNonShortCircuitLogic.cs | 33 ----------- .../DangerousNonShortCircuitLogic.expected | 4 -- .../DangerousNonShortCircuitLogic.qlref | 1 - 8 files changed, 2 insertions(+), 153 deletions(-) delete mode 100644 csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.cs delete mode 100644 csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.qhelp delete mode 100644 csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql delete mode 100644 csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogicFix.cs delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.expected delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.qlref diff --git a/change-notes/1.20/analysis-csharp.md b/change-notes/1.20/analysis-csharp.md index 13ec3648eb2e..ae5ce5856210 100644 --- a/change-notes/1.20/analysis-csharp.md +++ b/change-notes/1.20/analysis-csharp.md @@ -11,9 +11,10 @@ | *@name of query (Query ID)* | *Impact on results* | *How/why the query has changed* | |------------------------------|------------------------|-----------------------------------| -| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. | | Dereferenced variable is always null (cs/dereferenced-value-is-always-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. | | Dereferenced variable may be null (cs/dereferenced-value-may-be-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. | +| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. | +| Potentially dangerous use of non-short-circuit logic (cs/non-short-circuit) | No results | The query has been removed as it has been superseded by the queries Dereferenced variable is always null (cs/dereferenced-value-is-always-null) and Dereferenced variable may be null (cs/dereferenced-value-may-be-null). | ## Changes to code extraction diff --git a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.cs b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.cs deleted file mode 100644 index 276f4242a413..000000000000 --- a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.cs +++ /dev/null @@ -1,11 +0,0 @@ -class DangerousNonShortCircuitLogic -{ - public static void Main(string[] args) - { - string a = null; - if (a != null & a.ToLower() == "hello world") - { - Console.WriteLine("The string said hello world."); - } - } -} diff --git a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.qhelp b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.qhelp deleted file mode 100644 index cbbb13113cbc..000000000000 --- a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.qhelp +++ /dev/null @@ -1,35 +0,0 @@ - - - -

The | and & logical operators, known as non-short circuit -operators, should not be used. Using a non-short circuit operator reduces the efficiency of the -program, is potentially confusing and can even lead to the program crashing if the first operand -acts as a safety check for the second.

- - - -

If the non-short circuit operator is unintended then replace the operator with the short circuit -equivalent. Sometime a non-short circuit operator is required because the operands have side -effects. In this case it is more efficient to evaluate both operands separately and then use a -short circuit operator to combine the results.

- - - -

This example will crash because both parts of the conditional expression will be evaluated even -if a is null.

- - -

The example is easily fixed by using the short circuit AND operator. The program produces no -output but does not crash, unlike the previous example.

- - - - - -
  • MSDN: & Operator
    • -
  • MSDN: | Operator
    • - -     -     diff --git a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql deleted file mode 100644 index 48dea670ea93..000000000000 --- a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql +++ /dev/null @@ -1,57 +0,0 @@ -/** - * @name Potentially dangerous use of non-short-circuit logic - * @description The & and | operators do not use short-circuit evaluation and can be dangerous when applied to boolean operands. In particular, their - * use can result in errors if the left-hand operand checks for cases in which it is not safe to evaluate the right-hand one. - * @kind problem - * @problem.severity error - * @precision high - * @id cs/non-short-circuit - * @tags reliability - * correctness - * logic - * external/cwe/cwe-480 - * external/cwe/cwe-691 - */ - -import csharp - -/** An expression containing a qualified member access, a method call, or an array access. */ -class DangerousExpression extends Expr { - DangerousExpression() { - exists(Expr e | - this = e.getParent*() | - exists(Expr q | - q = e.(MemberAccess).getQualifier() | - not q instanceof ThisAccess - and - not q instanceof BaseAccess - ) - or - e instanceof MethodCall - or - e instanceof ArrayAccess - ) - } -} - -/** A use of `&` or `|` on operands of type boolean. */ -class NonShortCircuit extends BinaryBitwiseOperation { - NonShortCircuit() { - ( - this instanceof BitwiseAndExpr - or - this instanceof BitwiseOrExpr - ) - and - not exists(AssignBitwiseOperation abo | abo.getExpandedAssignment().getRValue() = this) - and - getLeftOperand().getType() instanceof BoolType - and - getRightOperand().getType() instanceof BoolType - and - getRightOperand() instanceof DangerousExpression - } -} - -from NonShortCircuit e -select e, "Potentially dangerous use of non-short circuit logic." diff --git a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogicFix.cs b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogicFix.cs deleted file mode 100644 index 8ee70b5b1b55..000000000000 --- a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogicFix.cs +++ /dev/null @@ -1,11 +0,0 @@ -class DangerousNonShortCircuitLogicFix -{ - public static void Main(string[] args) - { - string a = null; - if (a != null && a.ToLower() == "hello world") - { - Console.WriteLine("The string said hello world."); - } - } -} diff --git a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs b/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs deleted file mode 100644 index d52d45397f66..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs +++ /dev/null @@ -1,33 +0,0 @@ -class Test -{ - int Field; - - void M() - { - int i = 42; - var c = new C(); - - var x = i | Field; // GOOD - if (true & false) ; // GOOD - if (c != null ^ this.Field > 0) ; // GOOD - if (c != null && c.Field > 0) ; // GOOD - - if (c != null & c.Field > 0) ; // BAD - if (c == null | c.Property == "") ; // BAD - if (c == null | c[0]) ; // BAD - if (c == null | c.Method()) ; // BAD - - var b = true; - b &= c.Method(); // GOOD - b |= c[0]; // GOOD - } - - class C - { - public int Field; - public string Property { get; set; } - public bool this[int i] { get { return false; } set { } } - public bool Method() { return false; } - } -} - diff --git a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.expected b/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.expected deleted file mode 100644 index 8973461ca053..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.expected +++ /dev/null @@ -1,4 +0,0 @@ -| DangerousNonShortCircuitLogic.cs:15:13:15:35 | ... & ... | Potentially dangerous use of non-short circuit logic. | -| DangerousNonShortCircuitLogic.cs:16:13:16:40 | ... \| ... | Potentially dangerous use of non-short circuit logic. | -| DangerousNonShortCircuitLogic.cs:17:13:17:28 | ... \| ... | Potentially dangerous use of non-short circuit logic. | -| DangerousNonShortCircuitLogic.cs:18:13:18:34 | ... \| ... | Potentially dangerous use of non-short circuit logic. | diff --git a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.qlref b/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.qlref deleted file mode 100644 index 6e192b5b73f1..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.qlref +++ /dev/null @@ -1 +0,0 @@ -Likely Bugs/DangerousNonShortCircuitLogic.ql \ No newline at end of file