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

Name: Pointer offset used before it is checked

Description: Accessing a pointer or array using an offset before checking if the value is positive may result in unexpected behavior.

ID: cpp/late-negative-test

Kind: problem

Severity: warning

Query: LateNegativeTest.ql
/**
 * @name Pointer offset used before it is checked
 * @description Accessing a pointer or array using an offset before
 *              checking if the value is positive
 *              may result in unexpected behavior.
 * @kind problem
 * @id cpp/late-negative-test
 * @problem.severity warning
 * @tags reliability
 *       security
 *       external/cwe/cwe-823
 */

import cpp

predicate negativeCheck(LocalScopeVariable v, ComparisonOperation op) {
  exists(int varindex, string constant, Literal lit |
    op.getChild(varindex) = v.getAnAccess() and
    op.getChild(1 - varindex) = lit and
    lit.getValue() = constant and
    (
      op.getOperator() = "<" and varindex = 0 and constant = "0"
      or
      op.getOperator() = "<" and varindex = 1 and constant = "-1"
      or
      op.getOperator() = ">" and varindex = 0 and constant = "-1"
      or
      op.getOperator() = ">" and varindex = 1 and constant = "0"
      or
      op.getOperator() = "<=" and varindex = 0 and constant = "-1"
      or
      op.getOperator() = "<=" and varindex = 1 and constant = "0"
      or
      op.getOperator() = ">=" and varindex = 0 and constant = "0"
      or
      op.getOperator() = ">=" and varindex = 1 and constant = "-1"
    )
  )
}

from LocalScopeVariable v, ArrayExpr dangerous, Expr check
where
  useUsePair(v, dangerous.getArrayOffset(), check.getAChild()) and
  negativeCheck(v, check) and
  not exists(Expr other |
    negativeCheck(v, other) and useUsePair(v, other.getAChild(), dangerous.getArrayOffset())
  )
select dangerous,
  "Variable '" + v.getName() +
    "' is used as an array-offset before it is tested for being negative (test on line " +
    check.getLocation().getStartLine().toString() + "). "

This query finds integer values that are first used to index an array and subsequently tested for being negative. If it is relevant to perform this test at all then it should happen before the indexing, not after. Otherwise, if the value is negative then the program will have failed before performing the test.

This check is an approximation, so some results may not be actual defects in the program. It is not possible in general to compute the actual branch taken in conditional statements such as "if" without running the program with all possible input data. This means that it is not possible to determine if a particular statement is going to be executed.

Recommendation

See if the value needs to be checked before being used as array index. Double-check if the value is derived from user input. If the value clearly cannot be negative then the negativity test is redundant and can be removed.

Example

The example below includes two functions that use the value recordIdx to index an array and a test to verify that the value is positive. The test is made after records is indexed for printRecord and before records is indexed for processRecord. Unless the value of recordIdx cannot be negative, the test should be updated to run before both times the array is indexed. If the value cannot be negative, the test should be removed.

Record records[SIZE] = ...;

int f() {
	int recordIdx = 0;
	cin >> recordIdx;
	printRecord(&(records[recordIdx])); //incorrect: recordIdx may be negative here

	if (recordIdx >= 0) {
		processRecord(&(records[recordIdx])); //correct: index checked before use
	}
}

References