[clang-tidy] Warn about misuse of sizeof operator in loops.

[malavikasamak]

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

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Malavika Samak (malavikasamak)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/143205.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp (+16-1)
• (modified) clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h (+1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp (+31)

diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index f3d4c2255d86e..ee66a880792b8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -72,7 +72,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
           Options.get("WarnOnSizeOfPointerToAggregate", true)),
       WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
       WarnOnOffsetDividedBySizeOf(
-          Options.get("WarnOnOffsetDividedBySizeOf", true)) {}
+          Options.get("WarnOnOffsetDividedBySizeOf", true)),
+      WarnOnLoopExprSizeOf(Options.get("WarnOnLoopExprSizeOf", true)) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -86,6 +87,7 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
   Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
                 WarnOnOffsetDividedBySizeOf);
+  Options.store(Opts, "WarnOnLoopExprSizeOf", WarnOnLoopExprSizeOf);
 }
 
 void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -93,6 +95,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   // Some of the checks should not match in template code to avoid false
   // positives if sizeof is applied on template argument.
 
+  auto LoopExpr =
+      [](const ast_matchers::internal::Matcher<Stmt> &InnerMatcher) {
+        return stmt(anyOf(forStmt(InnerMatcher), whileStmt(InnerMatcher)));
+      };
+
   const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
   const auto ConstantExpr = ignoringParenImpCasts(
       anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
@@ -130,6 +137,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                        this);
   }
 
+  if (WarnOnLoopExprSizeOf) {
+    Finder->addMatcher(
+        LoopExpr(has(binaryOperator(has(SizeOfExpr)))).bind("loop-expr"), this);
+  }
+
   // Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
   const auto CharPtrType = pointerType(pointee(isAnyCharacter()));
   const auto ConstStrLiteralDecl =
@@ -353,6 +365,9 @@ 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")) {
+    diag(E->getBeginLoc(), "suspicious usage of 'sizeof' in the loop")
+        << E->getSourceRange();
   } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
     if (Result.Nodes.getNodeAs<Type>("struct-type")) {
       diag(E->getBeginLoc(),
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
index fbd62cb80fb2d..f7dccf39687a5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
+++ b/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 WarnOnLoopExprSizeOf;
 };
 
 } // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
index 5e6f394152e9d..52b71277466b1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -164,6 +164,37 @@ int Test2(MyConstChar* A) {
   return sum;
 }
 
+struct A {
+   int array[10];
+};
+
+struct B {
+  struct A a;
+};
+
+int square(int num, struct B b) {
+    struct A arr[10];
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+    for(int i = 0; i < sizeof(arr); i++) {
+       struct A a = arr[i];
+    }
+    // CHECK-MESSAGES: :[[@LINE+2]]:24: warning: suspicious usage of 'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression]
+    // CHECK-MESSAGES: :[[@LINE+1]]:34: 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];
+    }
+    
+    for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {
+       struct A a = arr[i];
+    }
+
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression]
+    for(int j = 0; j < sizeof(b.a); j++) {
+
+    }
+    return 2;
+}
+
 template <int T>
 int Foo() { int A[T]; return sizeof(T); }
 // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EugeneZelenko]

Please mention changes in Release Notes.

[HerrCai0907]

LGTM exception release note and doc.

[vbvictor]

Do we need this line? I think this argument is never printed because we don't have %0 is message

[malavikasamak]

The check here is ensuring the size of the array is not equal to the number of elements in the array. If they are equal, using the sizeof operator in the condition of the loop would be acceptable and unlikely to cause an out of bound access. I will add a comment explaining this.

[malavikasamak]

Hi Folks, If there is no more feedback, I am thinking of merging this PR today.