[clang-tidy] Make P +- BS / sizeof(*P) opt-outable in bugprone-sizeof-expression
#111178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CC @douzzer, who could not be added as a reviewer. |
|
@llvm/pr-subscribers-clang-tidy Author: None (whisperity) ChangesIn 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 This patch adds a new check option, Fixes #110551. Full diff: https://github.com/llvm/llvm-project/pull/111178.diff 6 Files Affected:
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;
+ }
+}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: None (whisperity) ChangesIn 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 This patch adds a new check option, Fixes #110551. Full diff: https://github.com/llvm/llvm-project/pull/111178.diff 6 Files Affected:
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;
+ }
+}
|
nicovank
approved these changes
Oct 5, 2024
...-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
Outdated
Show resolved
Hide resolved
nicovank
requested review from
PiotrZSL,
nicovank,
SimplyDanny,
5chmidti and
HerrCai0907
|
The first draft solution worked perfectly for me, tested via cherry pick. Will keep an eye on this PR in case the key name changes. Thanks @whisperity! |
5chmidti
reviewed
Oct 11, 2024
There was a problem hiding this comment.
Basically LGTM, but the name WarnOnArithmeticWithDivisionBySizeOf would imply that
(Buffer - Buffer) / sizeof(*Buffer);would be ignored as well when the flag is false. Please add something in the documentation about this. The option could also be renamed of course.
I don't really have a name that stands out as best, but here some ideas:
WarnOnOffsetByScaledDownSizeWarnOnOffsetDividedBySizeOf(this one comes closer to the above example again)
(non-blocking on renaming the option)
clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c
Show resolved
Hide resolved
5chmidti
approved these changes
Oct 12, 2024
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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) withsizeof(*Buffer). Since #106061, and as reported in #110551, this triggered a warning frombugprone-sizeof-expression, assizeofappeared 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.