Skip to content

[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression #94356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 11, 2024
Merged

[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression #94356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c94feff
[clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression
NagyDonat May 22, 2024
4de254d
Update the documentation and release notes
NagyDonat Jun 6, 2024
b3a7f8b
Use single back-ticks for option name
NagyDonat Jun 6, 2024
ce3a376
Generalize the suppression to all `sizeof(array[0])` expressions
NagyDonat Jun 7, 2024
d8e6228
Merge branch 'SizeofPointer-move-to-tidy' of github_via_443:Ericsson/…
NagyDonat Jun 7, 2024
ae8123a
Satisfy git-clang-format
NagyDonat Jun 7, 2024
59bd1b8
Extend `GenericFunctionTest`, document a class of FPs with a FIXME
NagyDonat Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 66 additions & 29 deletions clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &Opts) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
Expand All @@ -78,6 +79,7 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}

void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
Expand Down Expand Up @@ -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 ptr points to an aggregate (i.e. sizeof(&S)).
// Do not find it if RHS of a 'sizeof(arr) / sizeof(arr[0])' expression.
if (WarnOnSizeOfPointerToAggregate) {
// Detect sizeof(ptr) where ptr is a pointer (CWE-467).
//
// In WarnOnSizeOfPointerToAggregate mode only report cases when ptr points
// to an aggregate type or ptr is an expression that (implicitly or
// explicitly) casts an array to a pointer type. (These are more suspicious
// than other sizeof(ptr) expressions because they can appear as distorted
// forms of the common sizeof(aggregate) expressions.)
//
// To avoid false positives, some idiomatic constructs are accepted:
// + the RHS of a 'sizeof(arr) / sizeof(arr[0])' expression;
// + 'sizeof(*pp)' where 'pp' a pointer-to-pointer value, because this is
// a natural solution when dynamical typing is emulated by passing
// arguments as `generic_function(..., (void *)pp, sizeof(*pp))`.
// Moreover this generic message is suppressed in cases that are also matched
// by the more concrete matchers 'sizeof-this' and 'sizeof-charp'.
if (WarnOnSizeOfPointerToAggregate || WarnOnSizeOfPointer) {
const auto ArrayExpr =
ignoringParenImpCasts(hasType(hasCanonicalType(arrayType())));
const auto ArrayCastExpr = expr(anyOf(
Expand All @@ -149,8 +164,16 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {

const auto PointerToStructType =
hasUnqualifiedDesugaredType(pointerType(pointee(recordType())));
const auto PointerToStructExpr = expr(
hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr()));
const auto PointerToStructTypeWithBinding =
type(PointerToStructType).bind("struct-type");
const auto PointerToStructExpr =
expr(hasType(hasCanonicalType(PointerToStructType)));

const auto PointerToDetectedExpr =
WarnOnSizeOfPointer
? expr(hasType(hasUnqualifiedDesugaredType(pointerType())))
: expr(anyOf(ArrayCastExpr, PointerToArrayExpr,
PointerToStructExpr));

const auto ArrayOfPointersExpr = ignoringParenImpCasts(
hasType(hasCanonicalType(arrayType(hasElementType(pointerType()))
Expand All @@ -167,14 +190,17 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
hasLHS(ignoringParenImpCasts(sizeOfExpr(
has(ArrayOfPointersExpr)))))),
sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
const auto DerefExpr =
ignoringParenImpCasts(unaryOperator(hasOperatorName("*")));

Finder->addMatcher(
expr(sizeOfExpr(anyOf(
has(ignoringParenImpCasts(anyOf(
ArrayCastExpr, PointerToArrayExpr, PointerToStructExpr))),
has(PointerToStructType))),
expr(sizeOfExpr(anyOf(has(ignoringParenImpCasts(
expr(PointerToDetectedExpr, unless(DerefExpr),
unless(VarWithConstStrLiteralDecl),
unless(cxxThisExpr())))),
has(PointerToStructTypeWithBinding))),
unless(ArrayLengthExprDenom))
.bind("sizeof-pointer-to-aggregate"),
.bind("sizeof-pointer"),
this);
}

Expand Down Expand Up @@ -292,11 +318,17 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
} else if (const auto *E =
Result.Nodes.getNodeAs<Expr>("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<Expr>("sizeof-pointer")) {
if (Result.Nodes.getNodeAs<Type>("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")
<< E->getSourceRange();
}
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
"sizeof-compare-constant")) {
diag(E->getOperatorLoc(),
Expand Down Expand Up @@ -332,18 +364,23 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy == DenomTy) {
// FIXME: This message is wrong, it should not refer to sizeof "pointer"
// usage (and by the way, it would be to clarify all the messages).
diag(E->getOperatorLoc(),
"suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (!WarnOnSizeOfPointer) {
// When 'WarnOnSizeOfPointer' is enabled, these messages become redundant:
if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
}
}
} else if (const auto *E =
Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class SizeofExpressionCheck : public ClangTidyCheck {
const bool WarnOnSizeOfThis;
const bool WarnOnSizeOfCompareToConstant;
const bool WarnOnSizeOfPointerToAggregate;
const bool WarnOnSizeOfPointer;
};

} // namespace clang::tidy::bugprone
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
12 changes: 6 additions & 6 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,24 @@ int Test5() {

int sum = 0;
sum += sizeof(&S);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(__typeof(&S));
sum += sizeof(&TS);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(__typeof(&TS));
sum += sizeof(STRKWD MyStruct*);
sum += sizeof(__typeof(STRKWD MyStruct*));
sum += sizeof(TypedefStruct*);
sum += sizeof(__typeof(TypedefStruct*));
sum += sizeof(PTTS);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(PMyStruct);
sum += sizeof(PS);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(PS2);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(&A10);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer

#ifdef __cplusplus
MyStruct &rS = S;
Expand Down
Loading
Loading