CodeQL queries 1.24

Skip to end of metadata
Go to start of metadata

Name: Assignment where comparison was intended

Description: The '=' operator may have been used accidentally, where '==' was intended.

ID: cpp/assign-where-compare-meant

Kind: problem

Severity: error

Precision: high

Query: AssignWhereCompareMeant.ql
/**
 * @name Assignment where comparison was intended
 * @description The '=' operator may have been used accidentally, where '=='
 *              was intended.
 * @kind problem
 * @problem.severity error
 * @precision high
 * @id cpp/assign-where-compare-meant
 * @tags reliability
 *       correctness
 *       external/cwe/cwe-481
 */

import cpp
import semmle.code.cpp.controlflow.LocalScopeVariableReachability

class UndefReachability extends LocalScopeVariableReachability {
  UndefReachability() { this = "UndefReachability" }

  override predicate isSource(ControlFlowNode node, LocalScopeVariable v) {
    candidateVariable(v) and
    node = v.getParentScope() and
    not v instanceof Parameter and
    not v.hasInitializer()
  }

  override predicate isSink(ControlFlowNode node, LocalScopeVariable v) {
    candidateVariable(v) and
    node = v.getAnAccess()
  }

  override predicate isBarrier(ControlFlowNode node, LocalScopeVariable v) {
    node.(AssignExpr).getLValue() = v.getAnAccess()
  }
}

abstract class BooleanControllingAssignment extends AssignExpr {
  abstract predicate isWhitelisted();
}

class BooleanControllingAssignmentInExpr extends BooleanControllingAssignment {
  BooleanControllingAssignmentInExpr() {
    this.getParent() instanceof UnaryLogicalOperation or
    this.getParent() instanceof BinaryLogicalOperation or
    exists(ConditionalExpr c | c.getCondition() = this)
  }

  override predicate isWhitelisted() { this.getConversion().(ParenthesisExpr).isParenthesised() }
}

class BooleanControllingAssignmentInStmt extends BooleanControllingAssignment {
  BooleanControllingAssignmentInStmt() {
    exists(IfStmt i | i.getCondition() = this) or
    exists(ForStmt f | f.getCondition() = this) or
    exists(WhileStmt w | w.getCondition() = this) or
    exists(DoStmt d | d.getCondition() = this)
  }

  override predicate isWhitelisted() { this.isParenthesised() }
}

/**
 * Holds if `ae` is a `BooleanControllingAssignment` that would be a result of this query,
 * before checking for undef reachability.
 */
predicate candidateResult(BooleanControllingAssignment ae) {
  ae.getRValue().isConstant() and
  not ae.isWhitelisted()
}

/**
 * Holds if `v` is a `Variable` that might be assigned to in a result of this query.
 */
predicate candidateVariable(Variable v) {
  exists(BooleanControllingAssignment ae |
    candidateResult(ae) and
    ae.getLValue().(VariableAccess).getTarget() = v
  )
}

from BooleanControllingAssignment ae, UndefReachability undef
where
  candidateResult(ae) and
  not undef.reaches(_, ae.getLValue().(VariableAccess).getTarget(), ae.getLValue())
select ae, "Use of '=' where '==' may have been intended."

This rule finds uses of the assignment operator = in places where the equality operator == would make more sense. This is a very common mistake in C and C++, because of the similarity of the = and the == operator, and the fact that the if statement accepts a condition with an integral type, instead of limiting it to just the bool type.

The rule flags every occurrence of an assignment in a position where its result is interpreted as a truth value. An assignment is only flagged if its right hand side is a compile-time constant.

Recommendation

Check to ensure that the flagged expressions are not typos. If an assignment is really intended to be treated as a truth value, it may be better to surround it with parentheses.

Example

if(p = NULL) { //most likely == was intended. Otherwise it evaluates to the value
               //of the rhs of the assignment (which is NULL)
 ...
}

References