[clang-tidy] Make P +- BS / sizeof(*P) opt-outable in bugprone-sizeof-expression (llvm#111178)

In some cases and for projects that deal with a lot of low-level buffers, a
pattern often emerges that an array and its full size, not in the number of
"elements" but in "bytes", are known with no syntax-level connection between
the two values.
To access the array elements, the pointer arithmetic involved will have
to divide 'SizeInBytes' (a numeric value) with `sizeof(*Buffer)`.
Since the previous patch introduced this new warning, potential
false-positives were triggered from `bugprone-sizeof-expression`, as `sizeof`
appeared in pointer arithmetic where integers are scaled.

This patch adds a new check option, `WarnOnOffsetDividedBySizeOf`, which allows
users to opt out of warning about the division case.
In arbitrary projects, it might still be worthwhile to get these warnings until
an opt-out from the detection of scaling issues, especially if a project
might not be using low-level buffers intensively.

Author: whisperity

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

@@ -70,7 +70,9 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
           Options.get("WarnOnSizeOfCompareToConstant", true)),
       WarnOnSizeOfPointerToAggregate(
           Options.get("WarnOnSizeOfPointerToAggregate", true)),
-      WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
+      WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
+      WarnOnOffsetDividedBySizeOf(
+          Options.get("WarnOnOffsetDividedBySizeOf", true)) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -82,6 +84,8 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
                 WarnOnSizeOfPointerToAggregate);
   Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
+  Options.store(Opts, "WarnOnOffsetDividedBySizeOf",
+                WarnOnOffsetDividedBySizeOf);
 }
 
 void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -307,7 +311,8 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                  offsetOfExpr()))
           .bind("sizeof-in-ptr-arithmetic-scale-expr");
   const auto PtrArithmeticIntegerScaleExpr = binaryOperator(
-      hasAnyOperatorName("*", "/"),
+      WarnOnOffsetDividedBySizeOf ? binaryOperator(hasAnyOperatorName("*", "/"))
+                                  : binaryOperator(hasOperatorName("*")),
       // sizeof(...) * sizeof(...) and sizeof(...) / sizeof(...) is handled
       // by this check on another path.
       hasOperands(expr(hasType(isInteger()), unless(SizeofLikeScaleExpr)),

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

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

clang-tools-extra/docs/ReleaseNotes.rst

@@ -158,7 +158,7 @@ Changes in existing checks
 - 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.
+  subtracting from a pointer directly or when used to scale a numeric value.
 
 - Improved :doc:`bugprone-unchecked-optional-access
   <clang-tidy/checks/bugprone/unchecked-optional-access>` to support

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

@@ -221,6 +221,17 @@ 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.
 
+Similarly, multiplying or dividing a numeric value with the ``sizeof`` of an
+element or the whole buffer is suspicious, because the dimensional connection
+between the numeric value and the actual ``sizeof`` result can not always be
+deduced.
+While scaling an integer up (multiplying) with ``sizeof`` is likely **always**
+an issue, a scaling down (division) is not always inherently dangerous, in case
+the developer is aware that the division happens between an appropriate number
+of _bytes_ and a ``sizeof`` value.
+Turning :option:`WarnOnOffsetDividedBySizeOf` off will restrict the
+warnings to the multiplication case.
+
 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.
@@ -299,3 +310,9 @@ Options
    ``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`.
+
+.. option:: WarnOnOffsetDividedBySizeOf
+
+   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`.

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c

@@ -0,0 +1,14 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     bugprone-sizeof-expression.WarnOnOffsetDividedBySizeOf: false \
+// RUN:   }}'
+
+typedef __SIZE_TYPE__ size_t;
+
+void situational14(int *Buffer, size_t BufferSize) {
+  int *P = &Buffer[0];
+  while (P < Buffer + BufferSize / sizeof(*Buffer)) {
+    // NO-WARNING: This test opted out of "P +- N */ sizeof(...)" warnings.
+    ++P;
+  }
+}

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

@@ -352,21 +352,30 @@ void good13(void) {
   int Buffer[BufferSize];
 
   int *P = &Buffer[0];
-  while (P < (Buffer + sizeof(Buffer) / sizeof(int))) {
+  while (P < Buffer + sizeof(Buffer) / sizeof(int)) {
     // NO-WARNING: Calculating the element count of the buffer here, which is
     // safe with this idiom (as long as the types don't change).
     ++P;
   }
 
-  while (P < (Buffer + sizeof(Buffer) / sizeof(Buffer[0]))) {
+  while (P < Buffer + sizeof(Buffer) / sizeof(Buffer[0])) {
     // NO-WARNING: Calculating the element count of the buffer here, which is
     // safe with this idiom.
     ++P;
   }
 
-  while (P < (Buffer + sizeof(Buffer) / sizeof(*P))) {
+  while (P < Buffer + sizeof(Buffer) / sizeof(*P)) {
     // NO-WARNING: Calculating the element count of the buffer here, which is
     // safe with this idiom.
     ++P;
   }
 }
+
+void situational14(int *Buffer, size_t BufferSize) {
+  int *P = &Buffer[0];
+  while (P < Buffer + BufferSize / sizeof(*Buffer)) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic; this scaled value will be scaled again by the '+' operator
+    // CHECK-MESSAGES: :[[@LINE-2]]:21: note: '+' in pointer arithmetic internally scales with 'sizeof(int)' == {{[0-9]+}}
+    ++P;
+  }
+}