[clang-tools-extra] 1c38c46 - [clang-tidy] Make `P +- BS / sizeof(*P)` opt-outable in `bugprone-sizeof-expression` (#111178)

[via cfe-commits]

Author: whisperity
Date: 2024-10-15T14:42:57+02:00
New Revision: 1c38c46b083315e3a621267c9a90e8a7750f3700

URL: https://github.com/llvm/llvm-project/commit/1c38c46b083315e3a621267c9a90e8a7750f3700
DIFF: https://github.com/llvm/llvm-project/commit/1c38c46b083315e3a621267c9a90e8a7750f3700.diff

LOG: [clang-tidy] Make `P +- BS / sizeof(*P)` opt-outable in `bugprone-sizeof-expression` (#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.

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

Modified: 
    clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
    clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
    clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index a30e63f9b0fd6a..628d30ce7f73fe 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/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)),

diff  --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
index 66d7c34cc9e940..fbd62cb80fb2d0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
+++ b/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

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6196b9e15d3fc5..95be0a89cd6c93 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/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

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 89c88d2740dba2..29edb26ed7aa20 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
@@ -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`.

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c
new file mode 100644
index 00000000000000..b0cfb8d1cfa456
--- /dev/null
+++ b/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;
+  }
+}

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c
index 360ce00a6889d7..712141c5f266aa 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c
+++ b/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;
+  }
+}