[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
@@ -292,11 +318,17 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
- } else if (const auto *E =
- Result.Nodes.getNodeAs("sizeof-pointer-to-aggregate")) {
-diag(E->getBeginLoc(),
- "suspicious usage of 'sizeof(A*)'; pointer to aggregate")
-<< E->getSourceRange();
+ } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) {
+if (Result.Nodes.getNodeAs("struct-type")) {
+ diag(E->getBeginLoc(),
+ "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did
"
+ "you mean 'sizeof(A)'?")
+ << E->getSourceRange();
+} else {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
+ "that results in a pointer")
PiotrZSL wrote:
ok
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
=?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: https://github.com/PiotrZSL approved this pull request. Overall LGTM. It doesn't look to break (by default) previous behavior, so it's fine. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I evaluated the effects of the additional change [Generalize the suppression to all sizeof(array[0]) expressions](https://github.com/llvm/llvm-project/pull/94356/commits/ce3a37610228ceb9a9e60575f87886bf8e183493) compared to the earlier version of this PR, and it seems that this tweak eliminates a large amount of false positives: | Project | New Reports | Resolved Reports | |-|-|--| | memcached | No reports | No reports | tmux | No reports | No reports | curl | No reports | [3 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_previous_sizeofexpression_after_most_changes=curl_curl-7_66_0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | twin | No reports | No reports | vim | No reports | No reports | openssl | No reports | [8 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_previous_sizeofexpression_after_most_changes=openssl_openssl-3.0.0-alpha7_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | sqlite | No reports | [12 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_previous_sizeofexpression_after_most_changes=sqlite_version-3.33.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | ffmpeg | No reports | [9 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_previous_sizeofexpression_after_most_changes=ffmpeg_n4.3.1_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | postgres | No reports | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_previous_sizeofexpression_after_most_changes=postgres_REL_13_0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | tinyxml2 | No reports | No reports | libwebm | No reports | No reports | xerces | No reports | [10 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_previous_sizeofexpression_after_most_changes=xerces_v3.2.3_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | bitcoin | No reports | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_previous_sizeofexpression_after_most_changes=bitcoin_v0.20.1_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | protobuf | No reports | [4 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_previous_sizeofexpression_after_most_changes=protobuf_v3.13.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | qtbase | No reports | [7 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_previous_sizeofexpression_after_most_changes=qtbase_v6.2.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | contour | No reports | No reports | openrct2 | No reports | No reports | llvm-project | No reports | [6 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=llvm-project_llvmorg-12.0.0_previous_sizeofexpression_after_most_changes=llvm-project_llvmorg-12.0.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) I randomly checked ~20% of the resolved reports, and they all were false positives (that is, situations where `sizeof(pointer)` is the idiomatic and correct solution), so I'm satisfied with this additional change. At this point I'm satisfied with the behavior of this check on the real-world code and I'm not planning to add more features within this PR. (There is one known issue which I highlighted in a FIXME in commit [Extend GenericFunctionTest, document a class of FPs with a FIXME](https://github.com/llvm/llvm-project/pull/94356/commits/59bd1b83e4fa89b371f4d1a96c51fc7a1b4ad170) -- but that's significantly less severe than the issues before this PR, so I think it's enough to handle that later.) I also answered all the review suggestions, so I'd be grateful for another round of reviews from @PiotrZSL @EugeneZelenko or anybody else who has time for it. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/6] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/5] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I re-ran the open source evaluation, and here is the clean diff that I promised (italicized notes are just copied from the old table): | Project | New Reports | Resolved Reports | Notes |-|-|--|--| | memcached | No reports | No reports | – | tmux | No reports | [23 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_old_sizeofexpressions_with_new_messages=tmux_2.6_new_sizeofexpressions_rerun=Resolved) | _reports seem to be FPs, including several ones that [use `qsort` in a clear and straightforward way](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved=5493278=e1dd82bffcf68169ff8fe7181ca44f16=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Ftmux%2Fwindow-buffer.c)_ | curl | [3 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions_with_new_messages=curl_curl-7_66_0_new_sizeofexpressions_rerun=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions_with_new_messages=curl_curl-7_66_0_new_sizeofexpressions_rerun=Resolved) | _new reports are TPs (all reporting incorrect use of the same data structure), resolved one is FP_ | twin | No reports | No reports | – | vim | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_old_sizeofexpressions_with_new_messages=vim_v8.2.1920_new_sizeofexpressions_rerun=New) | No reports | _true positive_ | openssl | [23 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions_rerun_openssl_ffmpeg=New) | [22 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions_rerun_openssl_ffmpeg=Resolved) | resolved reports are FPs, new reports are mostly TPs or "works, but ugly and dodgy" code with a few FPs that look like `generic_function(, sizeof(arg))` or `get_memory(length*sizeof(array[0]))` | sqlite | [11 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions_with_new_messages=sqlite_version-3.33.0_new_sizeofexpressions_rerun=New) | No reports | _among the new results there are many FPs ([(1)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493379=f411835e93b1711c2889d4bef2889db9=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fshell.c), [(2)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493385=d9e3d0a984913130c821b7c18c2cc8d2=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fsqlite3.c)) that do things like `char **mem; realloc(mem, numElements*sizeof(mem[0]))`_ | ffmpeg | [22 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=ffmpeg_n4.3.1_new_sizeofexpressions_rerun_openssl_ffmpeg=New) | [109 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=ffmpeg_n4.3.1_new_sizeofexpressions_rerun_openssl_ffmpeg=Resolved) | postgres | No reports | [5 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions_with_new_messages=postgres_REL_13_0_new_sizeofexpressions_rerun=Resolved) | _resolved reports are FPs_ | tinyxml2 | No reports | No reports | – | libwebm | No reports | No reports | – | xerces | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_old_sizeofexpressions_with_new_messages=xerces_v3.2.3_new_sizeofexpressions_rerun=New) | No reports | true positive, seems to be an ugly bug | bitcoin | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_old_sizeofexpressions_with_new_messages=bitcoin_v0.20.1_new_sizeofexpressions_rerun=New) | No reports | false positive `hasher.Write((const unsigned char*), sizeof(ptr));` | protobuf | [5 new
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -190,6 +190,15 @@ Options .. option:: WarnOnSizeOfPointerToAggregate - When `true`, the check will warn on an expression like - ``sizeof(expr)`` where the expression is a pointer - to aggregate. Default is `true`. + When `true`, the check will warn when the argument of ``sizeof`` is either a + pointer-to-aggregate type, an expression returning a pointer-to-aggregate + value or an expression that returns a pointer from an array-to-pointer + conversion (that may be implicit or explicit, for example ``array + 2`` or + ``(int *)array``). Default is `true`. NagyDonat wrote: I also added a proper description for `WarnOnSizeOfPointerToAggregate`, because the old one was very vague and didn't explain the (somewhat complex) effects that are handled here. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/3] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: @@ -227,6 +227,12 @@ Changes in existing checks ` check by eliminating false positives resulting from use of optionals in unevaluated context. +- Improved :doc:`bugprone-sizeof-expression + ` check by eliminating some + false positives and adding a new (off-by-default) option + ``WarnOnSizeOfPointer`` that reports all ``sizeof(pointer)`` expressions EugeneZelenko wrote: ```suggestion `WarnOnSizeOfPointer` that reports all ``sizeof(pointer)`` expressions ``` Single back-ticks for option names and values. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/2] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -292,11 +318,17 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
- } else if (const auto *E =
- Result.Nodes.getNodeAs("sizeof-pointer-to-aggregate")) {
-diag(E->getBeginLoc(),
- "suspicious usage of 'sizeof(A*)'; pointer to aggregate")
-<< E->getSourceRange();
+ } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) {
+if (Result.Nodes.getNodeAs("struct-type")) {
+ diag(E->getBeginLoc(),
+ "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did
"
+ "you mean 'sizeof(A)'?")
+ << E->getSourceRange();
+} else {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
+ "that results in a pointer")
NagyDonat wrote:
I used this phrasing to be consistent with the analogous message "suspicious
usage of 'sizeof()' on an expression that results in an integer". I would also
prefer "suspicious use of 'sizeof()' on an expression of pointer type", but
then it would be good to update the message for integers, and then it would be
good to update all the other awkward messages printed by this check...
I'd prefer to keep the current consistent and (IMO) barely acceptable message
in this commit; and leave a general cleanup of all the messages of this checker
for a separate followup commit. (I'm not very enthusiastic for doing this
cleanup, but I can do it if the community feels that it would be important.)
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I analyzed several open source project with this check to observe the effects of my commit and I was (pleasantly?) surprised to see that it detected some ugly errors (despite the fact that the inputs are stable open source projects...). The following table compares the "old" state before this commit and the "new" state where this commit is active and the freshly added option `WarnOnSizeOfPointer` is set to true (instead of the default "false"): | Project | New Reports | Resolved Reports | Notes | |-|-|--|---| | memcached | No reports | No reports | – | tmux | No reports | [23 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved) | reports seem to be FPs, including several ones that [use `qsort` in a clear and straightforward way](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved=5493278=e1dd82bffcf68169ff8fe7181ca44f16=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Ftmux%2Fwindow-buffer.c) | curl | [3 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions=curl_curl-7_66_0_new_sizeofexpressions=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions=curl_curl-7_66_0_new_sizeofexpressions=Resolved) | new reports are TPs (all reporting incorrect use of the same data structure), resolved one is FP | twin | No reports | No reports | – | vim | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_old_sizeofexpressions=vim_v8.2.1920_new_sizeofexpressions=New) | No reports | true positive | openssl | [33 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions=New) | [32 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions=Resolved) | ... | sqlite | [19 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New) | [8 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=Resolved) | among the new results there are many FPs ([(1)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493379=f411835e93b1711c2889d4bef2889db9=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fshell.c), [(2)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493385=d9e3d0a984913130c821b7c18c2cc8d2=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fsqlite3.c)) that do things like `char **mem; realloc(mem, numElements*sizeof(mem[0]))`; the resolved reports are FPs and they reappear among the new reports with a different message | ffmpeg | [31 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions=ffmpeg_n4.3.1_new_sizeofexpressions=New) | [118 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions=ffmpeg_n4.3.1_new_sizeofexpressions=Resolved) | ... | postgres | [2 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions=postgres_REL_13_0_new_sizeofexpressions=New) | [7 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions=postgres_REL_13_0_new_sizeofexpressions=Resolved) | resolved reports are mostly FPs, two of them (one FP + one "technically works, but stupid" code) reappear as "new" reports with the new message | tinyxml2 | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions=tinyxml2_8.0.0_new_sizeofexpressions=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions=tinyxml2_8.0.0_new_sizeofexpressions=Resolved) | same FP is reported with the new message | libwebm | No reports | No reports | – | xerces | [16 new
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -193,3 +193,8 @@ Options When `true`, the check will warn on an expression like ``sizeof(expr)`` where the expression is a pointer to aggregate. Default is `true`. + +.. option:: WarnOnSizeOfPointer + + When `true`, the check will warn on an expression like ``sizeof(expr)`` + where the expression is a pointer. Default is `false`. PiotrZSL wrote: expression type is a pointer https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -292,11 +318,17 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
- } else if (const auto *E =
- Result.Nodes.getNodeAs("sizeof-pointer-to-aggregate")) {
-diag(E->getBeginLoc(),
- "suspicious usage of 'sizeof(A*)'; pointer to aggregate")
-<< E->getSourceRange();
+ } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) {
+if (Result.Nodes.getNodeAs("struct-type")) {
+ diag(E->getBeginLoc(),
+ "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did
"
+ "you mean 'sizeof(A)'?")
+ << E->getSourceRange();
+} else {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
+ "that results in a pointer")
PiotrZSL wrote:
maybe : "suspicious use of 'sizeof()' on an expression of pointer type" or even
"suspicious use of 'sizeof()' on a pointer type resulting from an expression"
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/PiotrZSL approved this pull request. +- LGTM, except nits. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -193,3 +193,8 @@ Options When `true`, the check will warn on an expression like ``sizeof(expr)`` where the expression is a pointer to aggregate. Default is `true`. + +.. option:: WarnOnSizeOfPointer + + When `true`, the check will warn on an expression like ``sizeof(expr)`` + where the expression is a pointer. Default is `false`. PiotrZSL wrote: You could mention here that CWE. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: @EugeneZelenko Thanks for inviting some additional reviewers. I'll add an entry in the release notes. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/EugeneZelenko commented: Please mention changes in Release Notes. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -124,8 +124,6 @@ int Test1(const char* ptr) {
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(P*)/sizeof(Q*)'
sum += sizeof(ptr) / sizeof(char);
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(T*)/sizeof(T)'
- sum += sizeof(ptr) / sizeof(ptr[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(T*)/sizeof(T)'
NagyDonat wrote:
I'm deleting this test line, because the same expression also appears on line
117.
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Donát Nagy (NagyDonat)
Changes
This commit reimplements the functionality of the Clang Static Analyzer checker
`alpha.core.SizeofPointer` within clang-tidy by adding a new (off-by-default)
option to bugprone-sizeof-expression which activates reporting all the
`sizeof(ptr)` expressions (where ptr is an expression that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer` was an
AST-based checker, which did not rely on the path sensitive capabilities of the
Static Analyzer, so there was no reason to keep it in the Static Analyzer
instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression, because
that check already provided several heuristics that reported various especially
suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't surprise the
existing users; but it can provide a more through coverage for the
vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than the existing
partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is a
pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped to avoid
several false positives on open-source code.
This commit also replaces the old message "suspicious usage of 'sizeof(A*)';
pointer to aggregate" with two more concrete messages; but I feel that this
tidy check would deserve a through cleanup of all the diagnostic messages that
it can produce. (I added a FIXME to mark one outright misleading message.)
---
Patch is 30.99 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/94356.diff
6 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
(+66-29)
- (modified) clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h (+1)
- (modified)
clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst (+5)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
(+6-6)
- (added)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
(+231)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
(+42-21)
``diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr) where
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/94356
This commit reimplements the functionality of the Clang Static Analyzer checker
`alpha.core.SizeofPointer` within clang-tidy by adding a new (off-by-default)
option to bugprone-sizeof-expression which activates reporting all the
`sizeof(ptr)` expressions (where ptr is an expression that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer` was an
AST-based checker, which did not rely on the path sensitive capabilities of the
Static Analyzer, so there was no reason to keep it in the Static Analyzer
instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression, because
that check already provided several heuristics that reported various especially
suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't surprise the
existing users; but it can provide a more through coverage for the
vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than the existing
partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is a
pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped to avoid
several false positives on open-source code.
This commit also replaces the old message "suspicious usage of 'sizeof(A*)';
pointer to aggregate" with two more concrete messages; but I feel that this
tidy check would deserve a through cleanup of all the diagnostic messages that
it can produce. (I added a FIXME to mark one outright misleading message.)
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
