[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 11 07:00:32 PDT 2024
https://github.com/whisperity updated https://github.com/llvm/llvm-project/pull/111178
>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 1/3] 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;
+ }
+}
>From fc90edd562bd43125791cb3e368184617707dc0d Mon Sep 17 00:00:00 2001
From: Whisperity <whisperity at gmail.com>
Date: Fri, 11 Oct 2024 15:42:16 +0200
Subject: [PATCH 2/3] fix: Rename the new option as per review request
---
.../bugprone/SizeofExpressionCheck.cpp | 17 +++++++----------
.../clang-tidy/bugprone/SizeofExpressionCheck.h | 2 +-
.../checks/bugprone/sizeof-expression.rst | 6 +++---
...expression-pointer-arithmetics-no-division.c | 4 +++-
4 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 4d09e8d0c59adb..fd9da6f4e1a6f7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -71,8 +71,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfPointerToAggregate(
Options.get("WarnOnSizeOfPointerToAggregate", true)),
WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)),
- WarnOnSizeOfPointerArithmeticWithDivisionScaled(Options.get(
- "WarnOnSizeOfPointerArithmeticWithDivisionScaled", true)) {}
+ WarnOnArithmeticWithDivisionBySizeOf(
+ Options.get("WarnOnArithmeticWithDivisionBySizeOf", true)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -84,8 +84,8 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
- Options.store(Opts, "WarnOnSizeOfPointerArithmeticWithDivisionScaled",
- WarnOnSizeOfPointerArithmeticWithDivisionScaled);
+ Options.store(Opts, "WarnOnArithmeticWithDivisionBySizeOf",
+ WarnOnArithmeticWithDivisionBySizeOf);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -310,13 +310,10 @@ 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(
- PtrArithmeticIntegerScaleExprInterestingOperatorNames(),
+ WarnOnArithmeticWithDivisionBySizeOf
+ ? 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 45a371a26f6eec..acf272df92bede 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
@@ -31,7 +31,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
const bool WarnOnSizeOfCompareToConstant;
const bool WarnOnSizeOfPointerToAggregate;
const bool WarnOnSizeOfPointer;
- const bool WarnOnSizeOfPointerArithmeticWithDivisionScaled;
+ const bool WarnOnArithmeticWithDivisionBySizeOf;
};
} // namespace clang::tidy::bugprone
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 e9a77a0c86b00a..16d5baef56913c 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
@@ -229,8 +229,8 @@ 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.
+Turning :option:`WarnOnArithmeticWithDivisionBySizeOf` 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,
@@ -311,7 +311,7 @@ Options
idiomatic expressions that are probably intentional and correct).
This detects occurrences of CWE 467. Default is `false`.
-.. option:: WarnOnSizeOfPointerArithmeticWithDivisionScaledInteger
+.. option:: WarnOnArithmeticWithDivisionBySizeOWarnOnArithmeticWithDivisionBySizeOf
When `true`, the check will warn on pointer arithmetic where the
element count is obtained from a division with ``sizeof(...)``,
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
index 5986f8c0ccf763..44a170c5462dc3 100644
--- 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
@@ -1,5 +1,7 @@
// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- \
-// RUN: -config="{CheckOptions: [{key: bugprone-sizeof-expression.WarnOnSizeOfPointerArithmeticWithDivisionScaled, value: 0}]}"
+// RUN: -config='{CheckOptions: { \
+// RUN: bugprone-sizeof-expression.WarnOnArithmeticWithDivisionBySizeOf: false \
+// RUN: }}'
typedef __SIZE_TYPE__ size_t;
>From 298ea450f663e240b60e1597e8a6d7d948790ced Mon Sep 17 00:00:00 2001
From: Whisperity <whisperity at gmail.com>
Date: Fri, 11 Oct 2024 16:00:13 +0200
Subject: [PATCH 3/3] fix: Documentation build fixed
---
.../docs/clang-tidy/checks/bugprone/sizeof-expression.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 16d5baef56913c..81c99d423e7863 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
@@ -311,7 +311,7 @@ Options
idiomatic expressions that are probably intentional and correct).
This detects occurrences of CWE 467. Default is `false`.
-.. option:: WarnOnArithmeticWithDivisionBySizeOWarnOnArithmeticWithDivisionBySizeOf
+.. option:: WarnOnArithmeticWithDivisionBySizeOf
When `true`, the check will warn on pointer arithmetic where the
element count is obtained from a division with ``sizeof(...)``,
More information about the cfe-commits
mailing list