CodeQL queries 1.25
Skip to end of metadata
Go to start of metadata

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.

ID: cs/non-short-circuit

Kind: problem

Severity: error

Precision: high

Query: DangerousNonShortCircuitLogic.ql
/**
 * @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
    ) and
    not exists(Expr e | this = e.getParent*() | e.(Call).getTarget().getAParameter().isOutOrRef())
  }
}

/** 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."

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.

Recommendation

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.

Example

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

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.");
        }
    }
}

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.

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.");
        }
    }
}

References