Skip to content

Commit 546c816

Browse files
NagyDonatNagyDonat
authored
[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression (#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. Previously this checker had an exception that the RHS of a `sizeof(array) / sizeof(array[0])` expression is not reported; I generalized this to an exception that the check doesn't report `sizeof(expr[0])` and `sizeof(*expr)`. This idea is taken from the Static Analyzer checker `alpha.core.SizeofPointer` (which had an exception for `*expr`), but analysis of open source projects confirmed that this indeed eliminates lots of unwanted results. Note that the suppression of `sizeof(expr[0])` and `sizeof(*expr)` reports also affects the "old" mode `WarnOnSizeOfPointerToAggregate` which is enabled by default. 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.)
1 parent ca920bb commit 546c816

File tree

7 files changed

+388
-75
lines changed

7 files changed

+388
-75
lines changed

‎clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp

+70-45
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
6767
WarnOnSizeOfCompareToConstant(
6868
Options.get("WarnOnSizeOfCompareToConstant", true)),
6969
WarnOnSizeOfPointerToAggregate(
70-
Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
70+
Options.get("WarnOnSizeOfPointerToAggregate", true)),
71+
WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
7172

7273
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
7374
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
7879
WarnOnSizeOfCompareToConstant);
7980
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
8081
WarnOnSizeOfPointerToAggregate);
82+
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
8183
}
8284

8385
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
127129
const auto ConstStrLiteralDecl =
128130
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
129131
hasInitializer(ignoringParenImpCasts(stringLiteral())));
132+
const auto VarWithConstStrLiteralDecl = expr(
133+
hasType(hasCanonicalType(CharPtrType)),
134+
ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl))));
130135
Finder->addMatcher(
131-
sizeOfExpr(has(ignoringParenImpCasts(
132-
expr(hasType(hasCanonicalType(CharPtrType)),
133-
ignoringParenImpCasts(declRefExpr(
134-
hasDeclaration(ConstStrLiteralDecl)))))))
136+
sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
135137
.bind("sizeof-charp"),
136138
this);
137139

138-
// Detect sizeof(ptr) where ptr points to an aggregate (i.e. sizeof(&S)).
139-
// Do not find it if RHS of a 'sizeof(arr) / sizeof(arr[0])' expression.
140-
if (WarnOnSizeOfPointerToAggregate) {
140+
// Detect sizeof(ptr) where ptr is a pointer (CWE-467).
141+
//
142+
// In WarnOnSizeOfPointerToAggregate mode only report cases when ptr points
143+
// to an aggregate type or ptr is an expression that (implicitly or
144+
// explicitly) casts an array to a pointer type. (These are more suspicious
145+
// than other sizeof(ptr) expressions because they can appear as distorted
146+
// forms of the common sizeof(aggregate) expressions.)
147+
//
148+
// To avoid false positives, the check doesn't report expressions like
149+
// 'sizeof(pp[0])' and 'sizeof(*pp)' where `pp` is a pointer-to-pointer or
150+
// array of pointers. (This filters out both `sizeof(arr) / sizeof(arr[0])`
151+
// expressions and other cases like `p = realloc(p, newsize * sizeof(*p));`.)
152+
//
153+
// Moreover this generic message is suppressed in cases that are also matched
154+
// by the more concrete matchers 'sizeof-this' and 'sizeof-charp'.
155+
if (WarnOnSizeOfPointerToAggregate || WarnOnSizeOfPointer) {
141156
const auto ArrayExpr =
142157
ignoringParenImpCasts(hasType(hasCanonicalType(arrayType())));
143158
const auto ArrayCastExpr = expr(anyOf(
@@ -149,32 +164,31 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
149164

150165
const auto PointerToStructType =
151166
hasUnqualifiedDesugaredType(pointerType(pointee(recordType())));
152-
const auto PointerToStructExpr = expr(
153-
hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr()));
154-
155-
const auto ArrayOfPointersExpr = ignoringParenImpCasts(
156-
hasType(hasCanonicalType(arrayType(hasElementType(pointerType()))
157-
.bind("type-of-array-of-pointers"))));
158-
const auto ArrayOfSamePointersExpr =
159-
ignoringParenImpCasts(hasType(hasCanonicalType(
160-
arrayType(equalsBoundNode("type-of-array-of-pointers")))));
167+
const auto PointerToStructTypeWithBinding =
168+
type(PointerToStructType).bind("struct-type");
169+
const auto PointerToStructExpr =
170+
expr(hasType(hasCanonicalType(PointerToStructType)));
171+
172+
const auto PointerToDetectedExpr =
173+
WarnOnSizeOfPointer
174+
? expr(hasType(hasUnqualifiedDesugaredType(pointerType())))
175+
: expr(anyOf(ArrayCastExpr, PointerToArrayExpr,
176+
PointerToStructExpr));
177+
161178
const auto ZeroLiteral = ignoringParenImpCasts(integerLiteral(equals(0)));
162-
const auto ArrayOfSamePointersZeroSubscriptExpr =
163-
ignoringParenImpCasts(arraySubscriptExpr(
164-
hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral)));
165-
const auto ArrayLengthExprDenom =
166-
expr(hasParent(binaryOperator(hasOperatorName("/"),
167-
hasLHS(ignoringParenImpCasts(sizeOfExpr(
168-
has(ArrayOfPointersExpr)))))),
169-
sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
179+
const auto SubscriptExprWithZeroIndex =
180+
arraySubscriptExpr(hasIndex(ZeroLiteral));
181+
const auto DerefExpr =
182+
ignoringParenImpCasts(unaryOperator(hasOperatorName("*")));
170183

171184
Finder->addMatcher(
172-
expr(sizeOfExpr(anyOf(
173-
has(ignoringParenImpCasts(anyOf(
174-
ArrayCastExpr, PointerToArrayExpr, PointerToStructExpr))),
175-
has(PointerToStructType))),
176-
unless(ArrayLengthExprDenom))
177-
.bind("sizeof-pointer-to-aggregate"),
185+
expr(sizeOfExpr(anyOf(has(ignoringParenImpCasts(
186+
expr(PointerToDetectedExpr, unless(DerefExpr),
187+
unless(SubscriptExprWithZeroIndex),
188+
unless(VarWithConstStrLiteralDecl),
189+
unless(cxxThisExpr())))),
190+
has(PointerToStructTypeWithBinding))))
191+
.bind("sizeof-pointer"),
178192
this);
179193
}
180194

@@ -292,11 +306,17 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
292306
diag(E->getBeginLoc(),
293307
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
294308
<< E->getSourceRange();
295-
} else if (const auto *E =
296-
Result.Nodes.getNodeAs<Expr>("sizeof-pointer-to-aggregate")) {
297-
diag(E->getBeginLoc(),
298-
"suspicious usage of 'sizeof(A*)'; pointer to aggregate")
299-
<< E->getSourceRange();
309+
} else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-pointer")) {
310+
if (Result.Nodes.getNodeAs<Type>("struct-type")) {
311+
diag(E->getBeginLoc(),
312+
"suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did "
313+
"you mean 'sizeof(A)'?")
314+
<< E->getSourceRange();
315+
} else {
316+
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
317+
"that results in a pointer")
318+
<< E->getSourceRange();
319+
}
300320
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
301321
"sizeof-compare-constant")) {
302322
diag(E->getOperatorLoc(),
@@ -332,18 +352,23 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
332352
" numerator is not a multiple of denominator")
333353
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
334354
} else if (NumTy && DenomTy && NumTy == DenomTy) {
355+
// FIXME: This message is wrong, it should not refer to sizeof "pointer"
356+
// usage (and by the way, it would be to clarify all the messages).
335357
diag(E->getOperatorLoc(),
336358
"suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
337359
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
338-
} else if (PointedTy && DenomTy && PointedTy == DenomTy) {
339-
diag(E->getOperatorLoc(),
340-
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
341-
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
342-
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
343-
DenomTy->isPointerType()) {
344-
diag(E->getOperatorLoc(),
345-
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
346-
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
360+
} else if (!WarnOnSizeOfPointer) {
361+
// When 'WarnOnSizeOfPointer' is enabled, these messages become redundant:
362+
if (PointedTy && DenomTy && PointedTy == DenomTy) {
363+
diag(E->getOperatorLoc(),
364+
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
365+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
366+
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
367+
DenomTy->isPointerType()) {
368+
diag(E->getOperatorLoc(),
369+
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
370+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
371+
}
347372
}
348373
} else if (const auto *E =
349374
Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {

‎clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h

+1
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
3030
const bool WarnOnSizeOfThis;
3131
const bool WarnOnSizeOfCompareToConstant;
3232
const bool WarnOnSizeOfPointerToAggregate;
33+
const bool WarnOnSizeOfPointer;
3334
};
3435

3536
} // namespace clang::tidy::bugprone

‎clang-tools-extra/docs/ReleaseNotes.rst

+6
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,12 @@ Changes in existing checks
237237
<clang-tidy/checks/bugprone/optional-value-conversion>` check by eliminating
238238
false positives resulting from use of optionals in unevaluated context.
239239

240+
- Improved :doc:`bugprone-sizeof-expression
241+
<clang-tidy/checks/bugprone/sizeof-expression>` check by eliminating some
242+
false positives and adding a new (off-by-default) option
243+
`WarnOnSizeOfPointer` that reports all ``sizeof(pointer)`` expressions
244+
(except for a few that are idiomatic).
245+
240246
- Improved :doc:`bugprone-suspicious-include
241247
<clang-tidy/checks/bugprone/suspicious-include>` check by replacing the local
242248
options `HeaderFileExtensions` and `ImplementationFileExtensions` by the

‎clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst

+12-3
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,15 @@ Options
190190

191191
.. option:: WarnOnSizeOfPointerToAggregate
192192

193-
When `true`, the check will warn on an expression like
194-
``sizeof(expr)`` where the expression is a pointer
195-
to aggregate. Default is `true`.
193+
When `true`, the check will warn when the argument of ``sizeof`` is either a
194+
pointer-to-aggregate type, an expression returning a pointer-to-aggregate
195+
value or an expression that returns a pointer from an array-to-pointer
196+
conversion (that may be implicit or explicit, for example ``array + 2`` or
197+
``(int *)array``). Default is `true`.
198+
199+
.. option:: WarnOnSizeOfPointer
200+
201+
When `true`, the check will report all expressions where the argument of
202+
``sizeof`` is an expression that produces a pointer (except for a few
203+
idiomatic expressions that are probably intentional and correct).
204+
This detects occurrences of CWE 467. Default is `false`.

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c

+6-6
Original file line numberDiff line numberDiff line change
@@ -34,24 +34,24 @@ int Test5() {
3434

3535
int sum = 0;
3636
sum += sizeof(&S);
37-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
37+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
3838
sum += sizeof(__typeof(&S));
3939
sum += sizeof(&TS);
40-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
40+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
4141
sum += sizeof(__typeof(&TS));
4242
sum += sizeof(STRKWD MyStruct*);
4343
sum += sizeof(__typeof(STRKWD MyStruct*));
4444
sum += sizeof(TypedefStruct*);
4545
sum += sizeof(__typeof(TypedefStruct*));
4646
sum += sizeof(PTTS);
47-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
47+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
4848
sum += sizeof(PMyStruct);
4949
sum += sizeof(PS);
50-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
50+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
5151
sum += sizeof(PS2);
52-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
52+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
5353
sum += sizeof(&A10);
54-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
54+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
5555

5656
#ifdef __cplusplus
5757
MyStruct &rS = S;

0 commit comments

Comments
 (0)