From 86026ee623cd9f02f4277a1f1ff0589b1b16fb30 Mon Sep 17 00:00:00 2001 From: Malavika Samak Date: Wed, 25 Jun 2025 10:04:10 -0700 Subject: [PATCH] [clang-tidy] Warn about misuse of sizeof operator in loops. (#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 --- .../bugprone/SizeofExpressionCheck.cpp | 38 ++++++++++- .../bugprone/SizeofExpressionCheck.h | 1 + clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ .../checks/bugprone/sizeof-expression.rst | 9 +++ .../checkers/bugprone/sizeof-expression.cpp | 63 +++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp index 9eeba867f521..e9ef0afc5ed6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ b/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,6 +88,8 @@ 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) { @@ -93,6 +97,13 @@ 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 LoopCondExpr = + [](const ast_matchers::internal::Matcher &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("loop-expr")) { + auto *SizeofArgTy = Result.Nodes.getNodeAs("sizeof-arg-type"); + if (const auto member = dyn_cast(SizeofArgTy)) + SizeofArgTy = member->getPointeeType().getTypePtr(); + + const auto *SzOfExpr = Result.Nodes.getNodeAs("sizeof-expr"); + + if (const auto type = dyn_cast(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("sizeof-pointer")) { diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression " "of pointer type") diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h index fbd62cb80fb2..e979b4723cf2 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h @@ -32,6 +32,7 @@ private: const bool WarnOnSizeOfPointerToAggregate; const bool WarnOnSizeOfPointer; const bool WarnOnOffsetDividedBySizeOf; + const bool WarnOnSizeOfInLoopTermination; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index fc51f3c9329a..934d52086b3b 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -173,6 +173,11 @@ Changes in existing checks ` check to detect conversion in argument of ``std::make_optional``. +- Improved :doc: `bugprone-sizeof-expression + ` check by adding + `WarnOnSizeOfInLoopTermination` option to detect misuses of ``sizeof`` + expression in loop conditions. + - Improved :doc:`bugprone-string-constructor ` check to find suspicious calls of ``std::string`` constructor with char pointer, start position and diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst index 29edb26ed7aa..04824cc1fe0e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst +++ b/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`. 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 5e6f394152e9..33cf1cbea837 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,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 Foo() { int A[T]; return sizeof(T); } // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'