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

…ession (llvm#94356)

This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).

The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.

After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.

It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.

The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.

Previously this checker had an exception that the RHS of a
`sizeof(array) / sizeof(array[0])` expression is not reported; I
generalized this to an exception that the check doesn't report
`sizeof(expr[0])` and `sizeof(*expr)`. This idea is taken from the
Static Analyzer checker `alpha.core.SizeofPointer` (which had an
exception for `*expr`), but analysis of open source projects confirmed
that this indeed eliminates lots of unwanted results.

Note that the suppression of `sizeof(expr[0])` and `sizeof(*expr)`
reports also affects the "old" mode `WarnOnSizeOfPointerToAggregate`
which is enabled by default.

This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)

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

@@ -237,6 +237,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;