github/codeql-coding-standards

`A4-7-1`: false positives in for loops

fjatWbyT opened this issue · 0 comments

Affected rules

  • A4-7-1

Description

Typical increment by one in for loops cannot lead to overflow or wrap-around because the number of iterations isn't large enough. The example illustrates true negatives together with false positives:

  • in f the variable used as "loop counter" is defined inside or outside the scope of the loop,
  • and in g the number of iterations changes by a factor 100, although still not close to int limit - in general I think a query should take into account, if possible, the value used to initialize the counter and the (potential maximum) number of iterations.
  • iterating through a container through an auxiliary span of its values (rather than through the container directly).

Example

#include <array>

void f() {
  const int num_iter = 100000;

  for (int i = 0; i < num_iter; i++) {  // OK.
    ;
  }

  int j = 0;
  for (; j < num_iter; j++) {  // False Positive.
    ;
  }
}

void g() {
  constexpr int N1000 = 1000;
  std::array<int, N1000> arr1000;
  std::size_t idx = 0;
  for (; idx < arr1000.size(); ++idx) {  // OK.
    ;
  }

  constexpr int N100000 = 100000;
  std::array<int, N100000> arr100000;
  idx = 0;
  for (; idx < arr100000.size(); ++idx) {  // False Positive.
    ;
  }
}

namespace {

// AUTOSAR is C++14, so a quick C++20's span:
template<typename T>
class span
{
 public:
  span(T* data, std::size_t s) : data_member(data), size_(s) {}
  std::size_t size() { return size_; }
 private:
  T* data_member;
  std::size_t size_;
};

// This could be a member (non-template) function in a custom (template) container.
template<typename Container>
constexpr span<typename Container::value_type> values(Container& c) {
  return {c.data(), c.size()};
}

}

int main() {
  constexpr int N = 1;
  std::array<int, N> arr;
  auto values = ::values(arr);
  std::size_t idx = 0;
  for (; idx < values.size(); ++idx) {  // False Positive.
    ;                                   // Avoid assuming N is declared in the same scope, but OK that it is a
                                        // std::size_t NTTP and/or data member of the container argument to values().
  }
}