[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression

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

@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
       WarnOnSizeOfCompareToConstant(
           Options.get("WarnOnSizeOfCompareToConstant", true)),
       WarnOnSizeOfPointerToAggregate(
-          Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+          Options.get("WarnOnSizeOfPointerToAggregate", true)),
+      WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
                 WarnOnSizeOfCompareToConstant);
   Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
                 WarnOnSizeOfPointerToAggregate);
+  Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
 }
 
 void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto ConstStrLiteralDecl =
       varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
               hasInitializer(ignoringParenImpCasts(stringLiteral())));
+  const auto VarWithConstStrLiteralDecl = expr(
+      hasType(hasCanonicalType(CharPtrType)),
+      ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl))));
   Finder->addMatcher(
-      sizeOfExpr(has(ignoringParenImpCasts(
-                     expr(hasType(hasCanonicalType(CharPtrType)),
-                          ignoringParenImpCasts(declRefExpr(
-                              hasDeclaration(ConstStrLiteralDecl)))))))
+      sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
           .bind("sizeof-charp"),
       this);
 
-  // Detect sizeof(ptr) where ptr points to an aggregate (i.e. sizeof(&S)).
-  // Do not find it if RHS of a 'sizeof(arr) / sizeof(arr[0])' expression.
-  if (WarnOnSizeOfPointerToAggregate) {
+  // Detect sizeof(ptr) where ptr is a pointer (CWE-467).
+  //
+  // In WarnOnSizeOfPointerToAggregate mode only report cases when ptr points
+  // to an aggregate type or ptr is an expression that (implicitly or
+  // explicitly) casts an array to a pointer type. (These are more suspicious
+  // than other sizeof(ptr) expressions because they can appear as distorted
+  // forms of the common sizeof(aggregate) expressions.)
+  //
+  // To avoid false positives, the check doesn't report expressions like
+  // 'sizeof(pp[0])' and 'sizeof(*pp)' where `pp` is a pointer-to-pointer or
+  // array of pointers. (This filters out both `sizeof(arr) / sizeof(arr[0])`
+  // expressions and other cases like `p = realloc(p, newsize * sizeof(*p));`.)
+  //
+  // Moreover this generic message is suppressed in cases that are also matched
+  // by the more concrete matchers 'sizeof-this' and 'sizeof-charp'.
+  if (WarnOnSizeOfPointerToAggregate || WarnOnSizeOfPointer) {
     const auto ArrayExpr =
         ignoringParenImpCasts(hasType(hasCanonicalType(arrayType())));
     const auto ArrayCastExpr = expr(anyOf(
@@ -149,32 +164,31 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
 
     const auto PointerToStructType =
         hasUnqualifiedDesugaredType(pointerType(pointee(recordType())));
-    const auto PointerToStructExpr = expr(
-        hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr()));
-
-    const auto ArrayOfPointersExpr = ignoringParenImpCasts(
-        hasType(hasCanonicalType(arrayType(hasElementType(pointerType()))
-                                     .bind("type-of-array-of-pointers"))));
-    const auto ArrayOfSamePointersExpr =
-        ignoringParenImpCasts(hasType(hasCanonicalType(
-            arrayType(equalsBoundNode("type-of-array-of-pointers")))));
+    const auto PointerToStructTypeWithBinding =
+        type(PointerToStructType).bind("struct-type");
+    const auto PointerToStructExpr =
+        expr(hasType(hasCanonicalType(PointerToStructType)));
+
+    const auto PointerToDetectedExpr =
+        WarnOnSizeOfPointer
+            ? expr(hasType(hasUnqualifiedDesugaredType(pointerType())))
+            : expr(anyOf(ArrayCastExpr, PointerToArrayExpr,
+                         PointerToStructExpr));
+
     const auto ZeroLiteral = ignoringParenImpCasts(integerLiteral(equals(0)));
-    const auto ArrayOfSamePointersZeroSubscriptExpr =
-        ignoringParenImpCasts(arraySubscriptExpr(
-            hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral)));
-    const auto ArrayLengthExprDenom =
-        expr(hasParent(binaryOperator(hasOperatorName("/"),
-                                      hasLHS(ignoringParenImpCasts(sizeOfExpr(
-                                          has(ArrayOfPointersExpr)))))),
-             sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
+    const auto SubscriptExprWithZeroIndex =
+        arraySubscriptExpr(hasIndex(ZeroLiteral));
+    const auto DerefExpr =
+        ignoringParenImpCasts(unaryOperator(hasOperatorName("*")));
 
     Finder->addMatcher(
-        expr(sizeOfExpr(anyOf(
-                 has(ignoringParenImpCasts(anyOf(
-                     ArrayCastExpr, PointerToArrayExpr, PointerToStructExpr))),
-                 has(PointerToStructType))),
-             unless(ArrayLengthExprDenom))
-            .bind("sizeof-pointer-to-aggregate"),
+        expr(sizeOfExpr(anyOf(has(ignoringParenImpCasts(
+                                  expr(PointerToDetectedExpr, unless(DerefExpr),
+                                       unless(SubscriptExprWithZeroIndex),
+                                       unless(VarWithConstStrLiteralDecl),
+                                       unless(cxxThisExpr())))),
+                              has(PointerToStructTypeWithBinding))))
+            .bind("sizeof-pointer"),
         this);
   }
 
@@ -292,11 +306,17 @@ 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<Expr>("sizeof-pointer-to-aggregate")) {
-    diag(E->getBeginLoc(),
-         "suspicious usage of 'sizeof(A*)'; pointer to aggregate")
-        << E->getSourceRange();
+  } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
+    if (Result.Nodes.getNodeAs<Type>("struct-type")) {
+      diag(E->getBeginLoc(),
+           "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did "
+           "you mean 'sizeof(A)'?")
+          << E->getSourceRange();
+    } else {
+      diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
+                             "that results in a pointer")
+          << E->getSourceRange();
+    }
   } else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
                  "sizeof-compare-constant")) {
     diag(E->getOperatorLoc(),
@@ -332,18 +352,23 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
                                 " numerator is not a multiple of denominator")
           << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
     } else if (NumTy && DenomTy && NumTy == DenomTy) {
+      // FIXME: This message is wrong, it should not refer to sizeof "pointer"
+      // usage (and by the way, it would be to clarify all the messages).
       diag(E->getOperatorLoc(),
            "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
           << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-    } else if (PointedTy && DenomTy && PointedTy == DenomTy) {
-      diag(E->getOperatorLoc(),
-           "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
-          << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
-    } else if (NumTy && DenomTy && NumTy->isPointerType() &&
-               DenomTy->isPointerType()) {
-      diag(E->getOperatorLoc(),
-           "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
-          << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+    } else if (!WarnOnSizeOfPointer) {
+      // When 'WarnOnSizeOfPointer' is enabled, these messages become redundant:
+      if (PointedTy && DenomTy && PointedTy == DenomTy) {
+        diag(E->getOperatorLoc(),
+             "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
+            << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+      } else if (NumTy && DenomTy && NumTy->isPointerType() &&
+                 DenomTy->isPointerType()) {
+        diag(E->getOperatorLoc(),
+             "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
+            << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+      }
     }
   } else if (const auto *E =
                  Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {

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

@@ -30,6 +30,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
   const bool WarnOnSizeOfThis;
   const bool WarnOnSizeOfCompareToConstant;
   const bool WarnOnSizeOfPointerToAggregate;
+  const bool WarnOnSizeOfPointer;
 };
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

@@ -227,6 +227,12 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/optional-value-conversion>` check by eliminating
   false positives resulting from use of optionals in unevaluated context.
 
+- Improved :doc:`bugprone-sizeof-expression
+  <clang-tidy/checks/bugprone/sizeof-expression>` check by eliminating some
+  false positives and adding a new (off-by-default) option
+  `WarnOnSizeOfPointer` that reports all ``sizeof(pointer)`` expressions
+  (except for a few that are idiomatic).
+
 - Improved :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check by replacing the local
   options `HeaderFileExtensions` and `ImplementationFileExtensions` by the

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

@@ -190,6 +190,15 @@ Options
 
 .. option:: WarnOnSizeOfPointerToAggregate
 
-   When `true`, the check will warn on an expression like
-   ``sizeof(expr)`` where the expression is a pointer
-   to aggregate. Default is `true`.
+   When `true`, the check will warn when the argument of ``sizeof`` is either a
+   pointer-to-aggregate type, an expression returning a pointer-to-aggregate
+   value or an expression that returns a pointer from an array-to-pointer
+   conversion (that may be implicit or explicit, for example ``array + 2`` or
+   ``(int *)array``). Default is `true`.
+
+.. option:: WarnOnSizeOfPointer
+
+   When `true`, the check will report all expressions where the argument of
+   ``sizeof`` is an expression that produces a pointer (except for a few
+   idiomatic expressions that are probably intentional and correct).
+   This detects occurrences of CWE 467. Default is `false`.

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

@@ -34,24 +34,24 @@ int Test5() {
 
   int sum = 0;
   sum += sizeof(&S);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
   sum += sizeof(__typeof(&S));
   sum += sizeof(&TS);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
   sum += sizeof(__typeof(&TS));
   sum += sizeof(STRKWD MyStruct*);
   sum += sizeof(__typeof(STRKWD MyStruct*));
   sum += sizeof(TypedefStruct*);
   sum += sizeof(__typeof(TypedefStruct*));
   sum += sizeof(PTTS);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
   sum += sizeof(PMyStruct);
   sum += sizeof(PS);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
   sum += sizeof(PS2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
   sum += sizeof(&A10);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
 
 #ifdef __cplusplus
   MyStruct &rS = S;