[clang-tidy] Extend bugprone-sizeof-expression with matching `P +- …

…sizeof(T)` and `P +- N */ sizeof(T)` cases, add `cert-arr39-c` alias (llvm#106061) Improved `bugprone-sizeof-expression` check to find suspicious pointer arithmetic calculations where the pointer is offset by an `alignof()`, `offsetof()`, or `sizeof()` expression. Pointer arithmetic expressions implicitly scale the offset added to or subtracted from the address by the size of the pointee type. Using an offset expression that is already scaled by the size of the underlying type effectively results in a squared offset, which is likely an invalid pointer that points beyond the end of the intended array. ```c void printEveryEvenIndexElement(int *Array, size_t N) { int *P = Array; while (P <= Array + N * sizeof(int)) { // Suspicious pointer arithmetics using sizeof()! printf("%d ", *P); P += 2 * sizeof(int); // Suspicious pointer arithmetics using sizeof()! } } ``` --------- Co-authored-by: Whisperity <[email protected]>
david-arm · Sep 17, 2024 · 267ad43 · 267ad43
1 parent f4172f6
commit 267ad43
Show file tree

Hide file tree

Showing 12 changed files with 649 additions and 20 deletions.
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -48,6 +48,8 @@ AST_MATCHER_P2(Expr, hasSizeOfDescendant, int, Depth,
   return false;
 }
 
+AST_MATCHER(Expr, offsetOfExpr) { return isa<OffsetOfExpr>(Node); }
+
 CharUnits getSizeOfType(const ASTContext &Ctx, const Type *Ty) {
   if (!Ty || Ty->isIncompleteType() || Ty->isDependentType() ||
       isa<DependentSizedArrayType>(Ty) || !Ty->isConstantSizeType())
@@ -221,17 +223,15 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto ElemType =
       arrayType(hasElementType(recordType().bind("elem-type")));
   const auto ElemPtrType = pointerType(pointee(type().bind("elem-ptr-type")));
+  const auto SizeofDivideExpr = binaryOperator(
+      hasOperatorName("/"),
+      hasLHS(
+          ignoringParenImpCasts(sizeOfExpr(hasArgumentOfType(hasCanonicalType(
+              type(anyOf(ElemType, ElemPtrType, type())).bind("num-type")))))),
+      hasRHS(ignoringParenImpCasts(sizeOfExpr(
+          hasArgumentOfType(hasCanonicalType(type().bind("denom-type")))))));
 
-  Finder->addMatcher(
-      binaryOperator(
-          hasOperatorName("/"),
-          hasLHS(ignoringParenImpCasts(sizeOfExpr(hasArgumentOfType(
-              hasCanonicalType(type(anyOf(ElemType, ElemPtrType, type()))
-                                   .bind("num-type")))))),
-          hasRHS(ignoringParenImpCasts(sizeOfExpr(
-              hasArgumentOfType(hasCanonicalType(type().bind("denom-type")))))))
-          .bind("sizeof-divide-expr"),
-      this);
+  Finder->addMatcher(SizeofDivideExpr.bind("sizeof-divide-expr"), this);
 
   // Detect expression like: sizeof(...) * sizeof(...)); most likely an error.
   Finder->addMatcher(binaryOperator(hasOperatorName("*"),
@@ -257,8 +257,9 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                          .bind("sizeof-sizeof-expr"),
                      this);
 
-  // Detect sizeof in pointer arithmetic like: N * sizeof(S) == P1 - P2 or
-  // (P1 - P2) / sizeof(S) where P1 and P2 are pointers to type S.
+  // Detect sizeof usage in comparisons involving pointer arithmetics, such as
+  // N * sizeof(T) == P1 - P2 or (P1 - P2) / sizeof(T), where P1 and P2 are
+  // pointers to a type T.
   const auto PtrDiffExpr = binaryOperator(
       hasOperatorName("-"),
       hasLHS(hasType(hasUnqualifiedDesugaredType(pointerType(pointee(
@@ -285,6 +286,47 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
           hasRHS(ignoringParenImpCasts(SizeOfExpr.bind("sizeof-ptr-div-expr"))))
           .bind("sizeof-in-ptr-arithmetic-div"),
       this);
+
+  // SEI CERT ARR39-C. Do not add or subtract a scaled integer to a pointer.
+  // Detect sizeof, alignof and offsetof usage in pointer arithmetics where
+  // they are used to scale the numeric distance, which is scaled again by
+  // the pointer arithmetic operator. This can result in forming invalid
+  // offsets.
+  //
+  // Examples, where P is a pointer, N is some integer (both compile-time and
+  // run-time): P + sizeof(T), P + sizeof(*P), P + N * sizeof(*P).
+  //
+  // This check does not warn on cases where the pointee type is "1 byte",
+  // as those cases can often come from generics and also do not constitute a
+  // problem because the size does not affect the scale used.
+  const auto InterestingPtrTyForPtrArithmetic =
+      pointerType(pointee(qualType().bind("pointee-type")));
+  const auto SizeofLikeScaleExpr =
+      expr(anyOf(unaryExprOrTypeTraitExpr(ofKind(UETT_SizeOf)),
+                 unaryExprOrTypeTraitExpr(ofKind(UETT_AlignOf)),
+                 offsetOfExpr()))
+          .bind("sizeof-in-ptr-arithmetic-scale-expr");
+  const auto PtrArithmeticIntegerScaleExpr = binaryOperator(
+      hasAnyOperatorName("*", "/"),
+      // sizeof(...) * sizeof(...) and sizeof(...) / sizeof(...) is handled
+      // by this check on another path.
+      hasOperands(expr(hasType(isInteger()), unless(SizeofLikeScaleExpr)),
+                  SizeofLikeScaleExpr));
+  const auto PtrArithmeticScaledIntegerExpr =
+      expr(anyOf(SizeofLikeScaleExpr, PtrArithmeticIntegerScaleExpr),
+           unless(SizeofDivideExpr));
+
+  Finder->addMatcher(
+      expr(anyOf(
+          binaryOperator(hasAnyOperatorName("+", "-"),
+                         hasOperands(hasType(InterestingPtrTyForPtrArithmetic),
+                                     PtrArithmeticScaledIntegerExpr))
+              .bind("sizeof-in-ptr-arithmetic-plusminus"),
+          binaryOperator(hasAnyOperatorName("+=", "-="),
+                         hasLHS(hasType(InterestingPtrTyForPtrArithmetic)),
+                         hasRHS(PtrArithmeticScaledIntegerExpr))
+              .bind("sizeof-in-ptr-arithmetic-plusminus"))),
+      this);
 }
 
 void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
@@ -409,6 +451,43 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
           << SizeOfExpr->getSourceRange() << E->getOperatorLoc()
           << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
     }
+  } else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
+                 "sizeof-in-ptr-arithmetic-plusminus")) {
+    const auto *PointeeTy = Result.Nodes.getNodeAs<QualType>("pointee-type");
+    const auto *ScaleExpr =
+        Result.Nodes.getNodeAs<Expr>("sizeof-in-ptr-arithmetic-scale-expr");
+    const CharUnits PointeeSize = getSizeOfType(Ctx, PointeeTy->getTypePtr());
+    const int ScaleKind = [ScaleExpr]() {
+      if (const auto *UTTE = dyn_cast<UnaryExprOrTypeTraitExpr>(ScaleExpr))
+        switch (UTTE->getKind()) {
+        case UETT_SizeOf:
+          return 0;
+        case UETT_AlignOf:
+          return 1;
+        default:
+          return -1;
+        }
+
+      if (isa<OffsetOfExpr>(ScaleExpr))
+        return 2;
+
+      return -1;
+    }();
+
+    if (ScaleKind != -1 && PointeeSize > CharUnits::One()) {
+      diag(E->getExprLoc(),
+           "suspicious usage of '%select{sizeof|alignof|offsetof}0(...)' in "
+           "pointer arithmetic; this scaled value will be scaled again by the "
+           "'%1' operator")
+          << ScaleKind << E->getOpcodeStr() << ScaleExpr->getSourceRange();
+      diag(E->getExprLoc(),
+           "'%0' in pointer arithmetic internally scales with 'sizeof(%1)' == "
+           "%2",
+           DiagnosticIDs::Note)
+          << E->getOpcodeStr()
+          << PointeeTy->getAsString(Ctx.getPrintingPolicy())
+          << PointeeSize.getQuantity();
+    }
   }
 }
 

diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
@@ -13,7 +13,7 @@
 
 namespace clang::tidy::bugprone {
 
-/// Find suspicious usages of sizeof expression.
+/// Find suspicious usages of sizeof expressions.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/sizeof-expression.html

diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "../bugprone/ReservedIdentifierCheck.h"
 #include "../bugprone/SignalHandlerCheck.h"
 #include "../bugprone/SignedCharMisuseCheck.h"
+#include "../bugprone/SizeofExpressionCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
 #include "../bugprone/SuspiciousMemoryComparisonCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"
@@ -281,6 +282,9 @@ class CERTModule : public ClangTidyModule {
         "cert-oop58-cpp");
 
     // C checkers
+    // ARR
+    CheckFactories.registerCheck<bugprone::SizeofExpressionCheck>(
+        "cert-arr39-c");
     // CON
     CheckFactories.registerCheck<bugprone::SpuriouslyWakeUpFunctionsCheck>(
         "cert-con36-c");
@@ -332,6 +336,12 @@ class CERTModule : public ClangTidyModule {
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+    Opts["cert-arr39-c.WarnOnSizeOfConstant"] = "false";
+    Opts["cert-arr39-c.WarnOnSizeOfIntegerExpression"] = "false";
+    Opts["cert-arr39-c.WarnOnSizeOfThis"] = "false";
+    Opts["cert-arr39-c.WarnOnSizeOfCompareToConstant"] = "false";
+    Opts["cert-arr39-c.WarnOnSizeOfPointer"] = "false";
+    Opts["cert-arr39-c.WarnOnSizeOfPointerToAggregate"] = "false";
     Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
     Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions;
     Opts["cert-err33-c.AllowCastToVoid"] = "true";

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,10 @@ New checks
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
+- New alias :doc:`cert-arr39-c <clang-tidy/checks/cert/arr39-c>` to
+  :doc:`bugprone-sizeof-expression
+  <clang-tidy/checks/bugprone/sizeof-expression>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -115,14 +119,19 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/forwarding-reference-overload>` check by fixing
   a crash when determining if an ``enable_if[_t]`` was found.
 
-- Improved :doc:`cert-flp30-c<clang-tidy/checks/cert/flp30-c>` check to 
+- Improved :doc:`bugprone-sizeof-expression
+  <clang-tidy/checks/bugprone/sizeof-expression>` check to find suspicious
+  usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
+  subtracting from a pointer.
+
+- Improved :doc:`cert-flp30-c <clang-tidy/checks/cert/flp30-c>` check to
   fix false positive that floating point variable is only used in increment
   expression.
 
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
-  <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to avoid
-  false positive when member initialization depends on a structured binging
-  variable.
+  <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
+  avoid false positive when member initialization depends on a structured
+  binding variable.
 
 - Improved :doc:`misc-definitions-in-headers
   <clang-tidy/checks/misc/definitions-in-headers>` check by rewording the

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
@@ -164,6 +164,103 @@ hidden through macros.
     memcpy(dst, buf, sizeof(INT_SZ));  // sizeof(sizeof(int)) is suspicious.
   }
 
+Suspicious usages of 'sizeof(...)' in pointer arithmetic
+--------------------------------------------------------
+
+Arithmetic operators on pointers automatically scale the result with the size
+of the pointed typed.
+Further use of ``sizeof`` around pointer arithmetic will typically result in an
+unintended result.
+
+Scaling the result of pointer difference
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Subtracting two pointers results in an integer expression (of type
+``ptrdiff_t``) which expresses the distance between the two pointed objects in
+"number of objects between".
+A common mistake is to think that the result is "number of bytes between", and
+scale the difference with ``sizeof``, such as ``P1 - P2 == N * sizeof(T)``
+(instead of ``P1 - P2 == N``) or ``(P1 - P2) / sizeof(T)`` instead of
+``P1 - P2``.
+
+.. code-block:: c++
+
+  void splitFour(const Obj* Objs, size_t N, Obj Delimiter) {
+    const Obj *P = Objs;
+    while (P < Objs + N) {
+      if (*P == Delimiter) {
+        break;
+      }
+    }
+
+    if (P - Objs != 4 * sizeof(Obj)) { // Expecting a distance multiplied by sizeof is suspicious.
+      error();
+    }
+  }
+
+.. code-block:: c++
+
+  void iterateIfEvenLength(int *Begin, int *End) {
+    auto N = (Begin - End) / sizeof(int); // Dividing by sizeof() is suspicious.
+    if (N % 2)
+      return;
+
+    // ...
+  }
+
+Stepping a pointer with a scaled integer
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Conversely, when performing pointer arithmetics to add or subtract from a
+pointer, the arithmetic operator implicitly scales the value actually added to
+the pointer with the size of the pointee, as ``Ptr + N`` expects ``N`` to be
+"number of objects to step", and not "number of bytes to step".
+
+Seeing the calculation of a pointer where ``sizeof`` appears is suspicious,
+and the result is typically unintended, often out of bounds.
+``Ptr + sizeof(T)`` will offset the pointer by ``sizeof(T)`` elements,
+effectively exponentiating the scaling factor to the power of 2.
+
+This case also checks suspicious ``alignof`` and ``offsetof`` usages in
+pointer arithmetic, as both return the "size" in bytes and not elements,
+potentially resulting in doubly-scaled offsets.
+
+.. code-block:: c++
+
+  void printEveryEvenIndexElement(int *Array, size_t N) {
+    int *P = Array;
+    while (P <= Array + N * sizeof(int)) { // Suspicious pointer arithmetic using sizeof()!
+      printf("%d ", *P);
+
+      P += 2 * sizeof(int); // Suspicious pointer arithmetic using sizeof()!
+    }
+  }
+
+.. code-block:: c++
+
+  struct Message { /* ... */; char Flags[8]; };
+  void clearFlags(Message *Array, size_t N) {
+    const Message *End = Array + N;
+    while (Array < End) {
+      memset(Array + offsetof(Message, Flags), // Suspicious pointer arithmetic using offsetof()!
+             0, sizeof(Message::Flags));
+      ++Array;
+    }
+  }
+
+For this checked bogus pattern, `cert-arr39-c` redirects here as an alias of
+this check.
+
+This check corresponds to the CERT C Coding Standard rule
+`ARR39-C. Do not add or subtract a scaled integer to a pointer
+<http://wiki.sei.cmu.edu/confluence/display/c/ARR39-C.+Do+not+add+or+subtract+a+scaled+integer+to+a+pointer>`_.
+
+Limitations
+"""""""""""
+
+Cases where the pointee type has a size of `1` byte (such as, and most
+importantly, ``char``) are excluded.
+
 Options
 -------
 

diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert/arr39-c.rst b/clang-tools-extra/docs/clang-tidy/checks/cert/arr39-c.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-arr39-c
+.. meta::
+   :http-equiv=refresh: 5;URL=../bugprone/sizeof-expression.html
+
+cert-arr39-c
+============
+
+The `cert-arr39-c` check is an alias, please see
+:doc:`bugprone-sizeof-expression <../bugprone/sizeof-expression>`
+for more information.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -407,6 +407,7 @@ Check aliases
    :header: "Name", "Redirect", "Offers fixes"
 
    :doc:`bugprone-narrowing-conversions <bugprone/narrowing-conversions>`, :doc:`cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions>`,
+   :doc:`cert-arr39-c <cert/arr39-c>`, :doc:`bugprone-sizeof-expression <bugprone/sizeof-expression>`,
    :doc:`cert-con36-c <cert/con36-c>`, :doc:`bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions>`,
    :doc:`cert-con54-cpp <cert/con54-cpp>`, :doc:`bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions>`,
    :doc:`cert-ctr56-cpp <cert/ctr56-cpp>`, :doc:`bugprone-pointer-arithmetic-on-polymorphic-object <bugprone/pointer-arithmetic-on-polymorphic-object>`,

diff --git a/...tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-c11.c b/...tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-c11.c
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy -std=c11-or-later %s bugprone-sizeof-expression %t
+
+#define alignof(type_name) _Alignof(type_name)
+extern void sink(const void *P);
+
+enum { BufferSize = 1024 };
+
+struct S {
+  long A, B, C;
+};
+
+void bad4d(void) {
+  struct S Buffer[BufferSize];
+
+  struct S *P = &Buffer[0];
+  struct S *Q = P;
+  while (Q < P + alignof(Buffer)) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: suspicious usage of 'alignof(...)' in pointer arithmetic; this scaled value will be scaled again by the '+' operator [bugprone-sizeof-expression]
+    // CHECK-MESSAGES: :[[@LINE-2]]:16: note: '+' in pointer arithmetic internally scales with 'sizeof(struct S)' == {{[0-9]+}}
+    sink(Q++);
+  }
+}