[clang-tidy] Warn about misuse of sizeof operator in loops. (llvm#143205)

The sizeof operator misuses in loop conditionals can be a source of
bugs. The common misuse is attempting to retrieve the number of elements
in the array by using the sizeof expression and forgetting to divide the
value by the sizeof the array elements. This results in an incorrect
computation of the array length and requires a warning from the sizeof
checker.

Example:
```
 int array[20];

void test_for_loop() {
  // Needs warning.
  for(int i = 0; i < sizeof(array); i++) {
    array[i] = i;
  }
}

void test_while_loop() {

  int count = 0;
  // Needs warning. 
  while(count < sizeof(array)) {
    array[count] = 0;
    count = count + 2;
  }
}
```
rdar://151403083

---------

Co-authored-by: MalavikaSamak <[email protected]>

Author: 2 people

clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp

@@ -72,7 +72,9 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
           Options.get("WarnOnSizeOfPointerToAggregate", true)),
       WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
       WarnOnOffsetDividedBySizeOf(
-          Options.get("WarnOnOffsetDividedBySizeOf", true)) {}
+          Options.get("WarnOnOffsetDividedBySizeOf", true)),
+      WarnOnSizeOfInLoopTermination(
+          Options.get("WarnOnSizeOfInLoopTermination", true)) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -86,13 +88,22 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
   Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
                 WarnOnOffsetDividedBySizeOf);
+  Options.store(Opts, "WarnOnSizeOfInLoopTermination",
+                WarnOnSizeOfInLoopTermination);
 }
 
 void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   // FIXME:
   // Some of the checks should not match in template code to avoid false
   // positives if sizeof is applied on template argument.
 
+  auto LoopCondExpr =
+      [](const ast_matchers::internal::Matcher<Stmt> &InnerMatcher) {
+        return stmt(anyOf(forStmt(hasCondition(InnerMatcher)),
+                          whileStmt(hasCondition(InnerMatcher)),
+                          doStmt(hasCondition(InnerMatcher))));
+      };
+
   const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
   const auto ConstantExpr = ignoringParenImpCasts(
       anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
@@ -130,6 +141,14 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                        this);
   }
 
+  if (WarnOnSizeOfInLoopTermination) {
+    auto CondExpr = binaryOperator(
+        allOf(has(SizeOfExpr.bind("sizeof-expr")), isComparisonOperator()));
+    Finder->addMatcher(LoopCondExpr(anyOf(CondExpr, hasDescendant(CondExpr)))
+                           .bind("loop-expr"),
+                       this);
+  }
+
   // Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
   const auto CharPtrType = pointerType(pointee(isAnyCharacter()));
   const auto ConstStrLiteralDecl =
@@ -349,6 +368,23 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
     diag(E->getBeginLoc(),
          "suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
         << E->getSourceRange();
+  } else if (const auto *E = Result.Nodes.getNodeAs<Stmt>("loop-expr")) {
+    auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
+    if (const auto member = dyn_cast<MemberPointerType>(SizeofArgTy))
+      SizeofArgTy = member->getPointeeType().getTypePtr();
+
+    const auto *SzOfExpr = Result.Nodes.getNodeAs<Expr>("sizeof-expr");
+
+    if (const auto type = dyn_cast<ArrayType>(SizeofArgTy)) {
+      // check if the array element size is larger than one. If true,
+      // the size of the array is higher than the number of elements
+      CharUnits sSize = Ctx.getTypeSizeInChars(type->getElementType());
+      if (!sSize.isOne()) {
+        diag(SzOfExpr->getBeginLoc(),
+             "suspicious usage of 'sizeof' in the loop")
+            << SzOfExpr->getSourceRange();
+      }
+    }
   } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
     diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
                            "of pointer type")

clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h

@@ -32,6 +32,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
   const bool WarnOnSizeOfPointerToAggregate;
   const bool WarnOnSizeOfPointer;
   const bool WarnOnOffsetDividedBySizeOf;
+  const bool WarnOnSizeOfInLoopTermination;
 };
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

@@ -173,6 +173,11 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/optional-value-conversion>` check to detect
   conversion in argument of ``std::make_optional``.
 
+- Improved :doc: `bugprone-sizeof-expression
+  <clang-tidy/checks/bugprone/bugprone-sizeof-expression>` check by adding
+  `WarnOnSizeOfInLoopTermination` option to detect misuses of ``sizeof``
+  expression in loop conditions.
+
 - Improved :doc:`bugprone-string-constructor
   <clang-tidy/checks/bugprone/string-constructor>` check to find suspicious
   calls of ``std::string`` constructor with char pointer, start position and

clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst

@@ -316,3 +316,12 @@ Options
    When `true`, the check will warn on pointer arithmetic where the
    element count is obtained from a division with ``sizeof(...)``,
    e.g., ``Ptr + Bytes / sizeof(*T)``. Default is `true`.
+
+.. option:: WarnOnSizeOfInLoopTermination
+
+   When `true`, the check will warn about incorrect use of sizeof expression
+   in loop termination condition. The warning triggers if the ``sizeof``
+   expression appears to be incorrectly used to determine the number of
+   array/buffer elements.
+   e.g, ``long arr[10]; for(int i = 0; i < sizeof(arr); i++) { ... }``. Default
+   is `true`.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp

@@ -164,6 +164,69 @@ int Test2(MyConstChar* A) {
   return sum;
 }
 
+struct A {
+   int array[10];
+};
+
+struct B {
+  struct A a;
+};
+
+void loop_access_elements(int num, struct B b) {
+  struct A arr[10];
+  char buf[20];
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:22: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  for(int i = 0; i < sizeof(arr); i++) {
+    struct A a = arr[i];
+  }
+
+  // Loop warning should not trigger here, even though this code is incorrect
+  // CHECK-MESSAGES: :[[@LINE+2]]:22: warning: suspicious usage of 'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression]
+  // CHECK-MESSAGES: :[[@LINE+1]]:32: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator [bugprone-sizeof-expression] 
+  for(int i = 0; i < sizeof(10)/sizeof(A); i++) {
+    struct A a = arr[i];
+  }
+    
+  // Should not warn here
+  for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {}
+
+  // Should not warn here
+  for (int i = 0; i < 10; i++) {
+    if (sizeof(arr) != 0) {
+
+    }
+  }
+
+  for (int i = 0; i < 10; i++) {
+    // CHECK-MESSAGES: :[[@LINE+1]]:25: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+    for (int j = 0; j < sizeof(arr); j++) {
+    }
+  }
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:22: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  for(int j = 0; j < sizeof(b.a.array); j++) {}
+  
+  // Should not warn here
+  for(int i = 0; i < sizeof(buf); i++) {} 
+
+  // Should not warn here
+  for(int i = 0; i < (sizeof(arr) << 3); i++) {}
+  
+  int i = 0;
+  // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  while(i <= sizeof(arr)) {i++;}
+   
+  i = 0;
+  do {
+    i++;
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] 
+  } while(i <= sizeof(arr));
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:29: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+  for(int i = 0, j = 0; i < sizeof(arr) && j < sizeof(buf); i++, j++) {}
+}
+
 template <int T>
 int Foo() { int A[T]; return sizeof(T); }
 // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'