Semmle 1.21
Skip to end of metadata
Go to start of metadata

Name: Comparison of narrow type with wide type in loop condition

Description: Comparisons between types of different widths in a loop condition can cause the loop to behave unexpectedly.

ID: cpp/comparison-with-wider-type

Kind: problem

Severity: warning

Precision: medium

Query: ComparisonWithWiderType.ql
/**
 * @name Comparison of narrow type with wide type in loop condition
 * @description Comparisons between types of different widths in a loop
 *              condition can cause the loop to behave unexpectedly.
 * @id cpp/comparison-with-wider-type
 * @kind problem
 * @problem.severity warning
 * @precision medium
 * @tags reliability
 *       security
 *       external/cwe/cwe-190
 *       external/cwe/cwe-197
 *       external/cwe/cwe-835
 * 
*/

import cpp
import semmle.code.cpp.controlflow.Dominance
import semmle.code.cpp.controlflow.SSA
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis

/**
 * C++ references are all pointer width, but the comparison takes place with
 * the pointed-to value
 */
int getComparisonSize(Expr e) {
  if e.getType() instanceof ReferenceType
  then result = e.getType().(ReferenceType).getBaseType().getSize()
  else result = e.getType().getSize()
}


predicate loopVariant(VariableAccess e, Loop loop) {
  exists (SsaDefinition d
    | d.getAUse(e.getTarget()) = e
    | d.getAnUltimateDefiningValue(e.getTarget()) = loop.getCondition().getAChild*()
    or d.getAnUltimateDefiningValue(e.getTarget()).getEnclosingStmt().getParent*()
      = loop.getStmt()
    or d.getAnUltimateDefiningValue(e.getTarget())
      = loop.(ForStmt).getUpdate().getAChild*())
}

Element friendlyLoc(Expr e) {
  result = e.(Access).getTarget() or
  result = e.(Call).getTarget() or
  not e instanceof Access and not e instanceof Call and result = e
}

from Loop l, RelationalOperation rel, Expr small, Expr large
where small = rel.getLesserOperand()
  and large = rel.getGreaterOperand()
  and rel = l.getCondition().getAChild*()
  and upperBound(large).log2() > getComparisonSize(small) * 8
  // Ignore cases where the smaller type is int or larger
  // These are still bugs, but you should need a very large string or array to
  // trigger them. We will want to disable this for some applications, but it's
  // very noisy on codebases that started as 32-bit
  and small.getExplicitlyConverted().getType().getSize() < 4
  // Ignore cases where integer promotion has occurred on /, -, or >> expressions.
  and not getComparisonSize(large.(DivExpr).getLeftOperand()
    .getExplicitlyConverted()) <= getComparisonSize(small)
  and not getComparisonSize(large.(SubExpr).getLeftOperand()
    .getExplicitlyConverted()) <= getComparisonSize(small)
  and not getComparisonSize(large.(RShiftExpr).getLeftOperand()
    .getExplicitlyConverted()) <= getComparisonSize(small)
  // ignore loop-invariant smaller variables
  and loopVariant(small.getAChild*(), l)
select rel, "Comparison between $@ of type " + small.getType().getName() + " and $@ of wider type " + large.getType().getName() + ".", 
  friendlyLoc(small), small.toString(), friendlyLoc(large), large.toString()

In a loop condition, comparison of a value of a narrow type with a value of a wide type may result in unexpected behavior if the wider value is sufficiently large (or small). This is because the narrower value may overflow. This can lead to an infinite loop.

Recommendation

Change the types of the compared values so that the value on the narrower side of the comparison is at least as wide as the value it is being compared with.

Example

In this example, bytes_received is compared against max_get in a while loop. However, bytes_received is an int16_t, and max_get is an int32_t. Because max_get is larger than INT16_MAX, the loop condition is always true, so the loop never terminates.

This problem is avoided in the 'GOOD' case because bytes_received2 is an int32_t, which is as wide as the type of max_get.

void main(int argc, char **argv) {
	uint32_t big_num = INT32_MAX;
	char buf[big_num];
	int16_t bytes_received = 0;
	int max_get = INT16_MAX + 1;

	// BAD: 'bytes_received' is compared with a value of a wider type.
	// 'bytes_received' overflows before  reaching 'max_get',
	// causing an infinite loop
	while (bytes_received < max_get)
		bytes_received += get_from_input(buf, bytes_received);
	}

	uint32_t bytes_received = 0;

	// GOOD: 'bytes_received2' has a type  at least as wide as 'max_get'
	while (bytes_received < max_get) {
		bytes_received += get_from_input(buf, bytes_received);
	}

}


int getFromInput(char *buf, short pos) {
	// write to buf
	// ...
	return 1;
}

References