[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression by NagyDonat · Pull Request #94356 · llvm/llvm-project (original) (raw)

@llvm/pr-subscribers-clang-tidy

@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:

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(

void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant); @@ -78,6 +79,7 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { WarnOnSizeOfCompareToConstant); Options.store(Opts, "WarnOnSizeOfPointerToAggregate", WarnOnSizeOfPointerToAggregate);

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())));

@@ -167,14 +190,17 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { hasLHS(ignoringParenImpCasts(sizeOfExpr( has(ArrayOfPointersExpr)))))), sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));

@@ -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( "sizeof-compare-constant")) { diag(E->getOperatorLoc(), @@ -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) {

diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h index 55becdd4ecdba..9ca17bc9e6f12 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h @@ -30,6 +30,7 @@ class SizeofExpressionCheck : public ClangTidyCheck { const bool WarnOnSizeOfThis; const bool WarnOnSizeOfCompareToConstant; const bool WarnOnSizeOfPointerToAggregate;

} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst index c37df1706eb4e..8ddee3254c090 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 @@ -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 +

int sum = 0; sum += sizeof(&S);

#ifdef __cplusplus MyStruct &rS = S; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp new file mode 100644 index 0000000000000..98ad2b385de7d --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp @@ -0,0 +1,231 @@ +// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -config="{CheckOptions: {bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression: true, bugprone-sizeof-expression.WarnOnSizeOfPointer: true}}" -- + +class C {

+struct S { char a, b, c; }; + +enum E { E_VALUE = 0 }; +enum class EC { VALUE = 0 }; + +bool AsBool() { return false; } +int AsInt() { return 0; } +E AsEnum() { return E_VALUE; } +EC AsEnumClass() { return EC::VALUE; } +S AsStruct() { return {}; } + +struct M {