[clang-tools-extra] [clang-tidy] Make `P +- BS / sizeof(*P)` opt-outable in `bugprone-sizeof-expression` (PR #111178)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 4 08:48:29 PDT 2024
https://github.com/whisperity created https://github.com/llvm/llvm-project/pull/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. In order to access the array elements, the pointer arithmetic involved will have to divide `SizeInBytes` (a numeric value) with `sizeof(*Buffer)`. Since #106061, and as reported in #110551, this triggered a warning from `bugprone-sizeof-expression`, as `sizeof` appeared in pointer arithmetic where integers are scaled.
This patch adds a new check option, `WarnOnSizeOfPointerArithmeticWithDivisionScaled`, 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** in order to detect scaling issues, especially if a project might not be using low-level buffers intensively.
Fixes #110551.
>From 4415aaf903635d11cce7afc232e1768ad32e8592 Mon Sep 17 00:00:00 2001
From: Whisperity <whisperity at gmail.com>
Date: Fri, 4 Oct 2024 17:30:41 +0200
Subject: [PATCH] feat(clang-tidy): Make `P + BS / sizeof(*P)` opt-outable
---
.../bugprone/SizeofExpressionCheck.cpp | 13 +++++++++++--
.../clang-tidy/bugprone/SizeofExpressionCheck.h | 1 +
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checks/bugprone/sizeof-expression.rst | 17 +++++++++++++++++
...expression-pointer-arithmetics-no-division.c | 12 ++++++++++++
.../sizeof-expression-pointer-arithmetics.c | 15 ++++++++++++---
6 files changed, 54 insertions(+), 6 deletions(-)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index a30e63f9b0fd6a..4d09e8d0c59adb 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)),
+ WarnOnSizeOfPointerArithmeticWithDivisionScaled(Options.get(
+ "WarnOnSizeOfPointerArithmeticWithDivisionScaled", 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, "WarnOnSizeOfPointerArithmeticWithDivisionScaled",
+ WarnOnSizeOfPointerArithmeticWithDivisionScaled);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -306,8 +310,13 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
unaryExprOrTypeTraitExpr(ofKind(UETT_AlignOf)),
offsetOfExpr()))
.bind("sizeof-in-ptr-arithmetic-scale-expr");
+ const auto PtrArithmeticIntegerScaleExprInterestingOperatorNames = [this] {
+ if (WarnOnSizeOfPointerArithmeticWithDivisionScaled)
+ return binaryOperator(hasAnyOperatorName("*", "/"));
+ return binaryOperator(hasOperatorName("*"));
+ };
const auto PtrArithmeticIntegerScaleExpr = binaryOperator(
- hasAnyOperatorName("*", "/"),
+ PtrArithmeticIntegerScaleExprInterestingOperatorNames(),
// 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..45a371a26f6eec 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 WarnOnSizeOfPointerArithmeticWithDivisionScaled;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b4792d749a86c3..57b0d2ab243487 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -144,7 +144,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..e9a77a0c86b00a 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`` 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:`WarnOnSizeOfPointerArithmeticWithDivisionScaledInteger` 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:: WarnOnSizeOfPointerArithmeticWithDivisionScaledInteger
+
+ 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..5986f8c0ccf763
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c
@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- \
+// RUN: -config="{CheckOptions: [{key: bugprone-sizeof-expression.WarnOnSizeOfPointerArithmeticWithDivisionScaled, value: 0}]}"
+
+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;
+ }
+}
More information about the cfe-commits
mailing list