diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp index 5e64d23874ec1..c25ee42d0899a 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 &Opts) { Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant); @@ -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) { @@ -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, the check doesn't report expressions like + // 'sizeof(pp[0])' and 'sizeof(*pp)' where `pp` is a pointer-to-pointer or + // array of pointers. (This filters out both `sizeof(arr) / sizeof(arr[0])` + // expressions and other cases like `p = realloc(p, newsize * sizeof(*p));`.) + // + // 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( @@ -149,32 +164,31 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { const auto PointerToStructType = hasUnqualifiedDesugaredType(pointerType(pointee(recordType()))); - const auto PointerToStructExpr = expr( - hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr())); - - const auto ArrayOfPointersExpr = ignoringParenImpCasts( - hasType(hasCanonicalType(arrayType(hasElementType(pointerType())) - .bind("type-of-array-of-pointers")))); - const auto ArrayOfSamePointersExpr = - ignoringParenImpCasts(hasType(hasCanonicalType( - arrayType(equalsBoundNode("type-of-array-of-pointers"))))); + 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 ZeroLiteral = ignoringParenImpCasts(integerLiteral(equals(0))); - const auto ArrayOfSamePointersZeroSubscriptExpr = - ignoringParenImpCasts(arraySubscriptExpr( - hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral))); - const auto ArrayLengthExprDenom = - expr(hasParent(binaryOperator(hasOperatorName("/"), - hasLHS(ignoringParenImpCasts(sizeOfExpr( - has(ArrayOfPointersExpr)))))), - sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr))); + const auto SubscriptExprWithZeroIndex = + arraySubscriptExpr(hasIndex(ZeroLiteral)); + const auto DerefExpr = + ignoringParenImpCasts(unaryOperator(hasOperatorName("*"))); Finder->addMatcher( - expr(sizeOfExpr(anyOf( - has(ignoringParenImpCasts(anyOf( - ArrayCastExpr, PointerToArrayExpr, PointerToStructExpr))), - has(PointerToStructType))), - unless(ArrayLengthExprDenom)) - .bind("sizeof-pointer-to-aggregate"), + expr(sizeOfExpr(anyOf(has(ignoringParenImpCasts( + expr(PointerToDetectedExpr, unless(DerefExpr), + unless(SubscriptExprWithZeroIndex), + unless(VarWithConstStrLiteralDecl), + unless(cxxThisExpr())))), + has(PointerToStructTypeWithBinding)))) + .bind("sizeof-pointer"), this); } @@ -292,11 +306,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-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") + << E->getSourceRange(); + } } else if (const auto *E = Result.Nodes.getNodeAs( "sizeof-compare-constant")) { diag(E->getOperatorLoc(), @@ -332,18 +352,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("sizeof-sizeof-expr")) { 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; + const bool WarnOnSizeOfPointer; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 33b65caf2b02c..61e2b55a831e4 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -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 + (except for a few that are idiomatic). + - Improved :doc:`bugprone-suspicious-include ` check by replacing the local options `HeaderFileExtensions` and `ImplementationFileExtensions` by the 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..ed5bb4fbb89ba 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 @@ -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`. + +.. option:: WarnOnSizeOfPointer + + When `true`, the check will report all expressions where the argument of + ``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`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c index 8c4feb8f86169..aef930f2c8fda 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c @@ -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; 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..bfb2ec3a9eb02 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp @@ -0,0 +1,241 @@ +// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -config="{CheckOptions: {bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression: true, bugprone-sizeof-expression.WarnOnSizeOfPointer: true}}" -- + +class C { + int size() { return sizeof(this); } + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(this)' +}; + +#define LEN 8 + +int X; +extern int A[10]; +extern short B[10]; + +#pragma pack(1) +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 { + int AsInt() { return 0; } + E AsEnum() { return E_VALUE; } + S AsStruct() { return {}; } +}; + +int Test1(const char* ptr) { + int sum = 0; + sum += sizeof(LEN); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)' + sum += sizeof(LEN + 1); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)' + sum += sizeof(sum, LEN); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: suspicious usage of 'sizeof(..., ...)' + sum += sizeof(AsBool()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(AsInt()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(AsEnum()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(AsEnumClass()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(M{}.AsInt()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(M{}.AsEnum()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer + sum += sizeof(sizeof(X)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(LEN + sizeof(X)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(LEN + LEN + sizeof(X)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(LEN + (LEN + sizeof(X))); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(LEN + -sizeof(X)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(LEN + - + -sizeof(X)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))' + sum += sizeof(char) / sizeof(char); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)' + sum += sizeof(A) / sizeof(S); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator + sum += sizeof(char) / sizeof(int); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator + sum += sizeof(char) / sizeof(A); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator + sum += sizeof(B[0]) / sizeof(A); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator + sum += sizeof(ptr) / sizeof(char); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(ptr) / sizeof(ptr[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(ptr) / sizeof(char*); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(ptr) / sizeof(void*); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(ptr) / sizeof(const void volatile*); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(ptr) / sizeof(char); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(int) * sizeof(char); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication + sum += sizeof(ptr) * sizeof(ptr[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication + sum += sizeof(int) * (2 * sizeof(char)); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication + sum += (2 * sizeof(char)) * sizeof(int); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious 'sizeof' by 'sizeof' multiplication + if (sizeof(A) < 0x100000) sum += 42; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: suspicious comparison of 'sizeof(expr)' to a constant + if (sizeof(A) <= 0xFFFFFFFEU) sum += 42; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: suspicious comparison of 'sizeof(expr)' to a constant + return sum; +} + +int Test5() { + typedef int Array10[10]; + typedef C ArrayC[10]; + + struct MyStruct { + Array10 arr; + Array10* ptr; + }; + typedef const MyStruct TMyStruct; + typedef const MyStruct *PMyStruct; + typedef TMyStruct *PMyStruct2; + + static TMyStruct kGlocalMyStruct = {}; + static TMyStruct volatile * kGlocalMyStructPtr = &kGlocalMyStruct; + + MyStruct S; + PMyStruct PS; + PMyStruct2 PS2; + Array10 A10; + C *PtrArray[10]; + C *PC; + + char *PChar; + int *PInt, **PPInt; + MyStruct **PPMyStruct; + + int sum = 0; + sum += sizeof(&S.arr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(&kGlocalMyStruct.arr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(&kGlocalMyStructPtr->arr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(S.arr + 0); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(+ S.arr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof((int*)S.arr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + + sum += sizeof(S.ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(kGlocalMyStruct.ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(kGlocalMyStructPtr->ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + + sum += sizeof(&kGlocalMyStruct); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(&S); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(MyStruct*); + sum += sizeof(PMyStruct); + sum += sizeof(PS); + // 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()' on an expression that results in a pointer + sum += sizeof(&A10); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(PtrArray) / sizeof(PtrArray[1]); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(A10) / sizeof(PtrArray[0]); + sum += sizeof(PC) / sizeof(PtrArray[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)' + sum += sizeof(ArrayC) / sizeof(PtrArray[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator + + sum += sizeof(PChar); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(PInt); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(PPInt); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(PPMyStruct); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + + return sum; +} + +void some_generic_function(const void *arg, int argsize); +int *IntP, **IntPP; +C *ClassP, **ClassPP; + +void GenericFunctionTest() { + // The `sizeof(pointer)` checks ignore situations where the pointer is + // produced by dereferencing a pointer-to-pointer, because this is unlikely + // to be an accident and can appear in legitimate code that tries to call + // a generic function which emulates dynamic typing within C. + some_generic_function(IntPP, sizeof(*IntPP)); + some_generic_function(ClassPP, sizeof(*ClassPP)); + // Using `...[0]` instead of the dereference operator is another common + // variant, which is also widespread in the idiomatic array-size calculation: + // `sizeof(array) / sizeof(array[0])`. + some_generic_function(IntPP, sizeof(IntPP[0])); + some_generic_function(ClassPP, sizeof(ClassPP[0])); + // FIXME: There is a third common pattern where the generic function is + // called with `&Variable` and `sizeof(Variable)`. Right now these are + // reported by the `sizeof(pointer)` checks, but this causes some false + // positives, so it would be good to create an exception for them. + some_generic_function(&IntPP, sizeof(IntP)); + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + some_generic_function(&ClassPP, sizeof(ClassP)); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer +} + +int ValidExpressions() { + int A[] = {1, 2, 3, 4}; + static const char str[] = "hello"; + static const char* ptr[] { "aaa", "bbb", "ccc" }; + typedef C *CA10[10]; + C *PtrArray[10]; + CA10 PtrArray1; + + int sum = 0; + if (sizeof(A) < 10) + sum += sizeof(A); + sum += sizeof(int); + sum += sizeof(AsStruct()); + sum += sizeof(M{}.AsStruct()); + sum += sizeof(A[sizeof(A) / sizeof(int)]); + // Here the outer sizeof is reported, but the inner ones are accepted: + sum += sizeof(&A[sizeof(A) / sizeof(int)]); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer + sum += sizeof(sizeof(0)); // Special case: sizeof size_t. + sum += sizeof(void*); + sum += sizeof(void const *); + sum += sizeof(void const *) / 4; + sum += sizeof(str); + sum += sizeof(str) / sizeof(char); + sum += sizeof(str) / sizeof(str[0]); + sum += sizeof(ptr) / sizeof(ptr[0]); + sum += sizeof(ptr) / sizeof(*(ptr)); + sum += sizeof(PtrArray) / sizeof(PtrArray[0]); + // Canonical type of PtrArray1 is same as PtrArray. + sum = sizeof(PtrArray) / sizeof(PtrArray1[0]); + // There is no warning for 'sizeof(T*)/sizeof(Q)' case. + sum += sizeof(PtrArray) / sizeof(A[0]); + return sum; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp index 003a02209c3d2..064f31cb08c6b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp @@ -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)' sum += sizeof(int) * sizeof(char); // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious 'sizeof' by 'sizeof' multiplication sum += sizeof(ptr) * sizeof(ptr[0]); @@ -207,50 +205,57 @@ int Test5() { C *PtrArray[10]; C *PC; + char *PChar; + int *PInt, **PPInt; + MyStruct **PPMyStruct; + int sum = 0; sum += sizeof(&S.arr); - // 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(&kGlocalMyStruct.arr); - // 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(&kGlocalMyStructPtr->arr); - // 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(S.arr + 0); - // 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(+ S.arr); - // 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((int*)S.arr); - // 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(S.ptr); - // 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(kGlocalMyStruct.ptr); - // 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(kGlocalMyStructPtr->ptr); - // 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(&kGlocalMyStruct); - // 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(&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(MyStruct*); 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 sum += sizeof(PtrArray) / sizeof(PtrArray[1]); - // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer sum += sizeof(A10) / sizeof(PtrArray[0]); - // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate sum += sizeof(PC) / sizeof(PtrArray[0]); - // 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 // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)' - // CHECK-MESSAGES: :[[@LINE-3]]:23: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate sum += sizeof(ArrayC) / sizeof(PtrArray[0]); // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator - // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate + + // These pointers do not point to aggregate types, so they are not reported in this mode: + sum += sizeof(PChar); + sum += sizeof(PInt); + sum += sizeof(PPInt); + sum += sizeof(PPMyStruct); return sum; } @@ -293,6 +298,32 @@ bool Baz() { return sizeof(A) < N; } // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: suspicious comparison of 'sizeof(expr)' to a constant bool Test7() { return Baz<-1>(); } +void some_generic_function(const void *arg, int argsize); +int *IntP, **IntPP; +C *ClassP, **ClassPP; + +void GenericFunctionTest() { + // The `sizeof(pointer)` checks ignore situations where the pointer is + // produced by dereferencing a pointer-to-pointer, because this is unlikely + // to be an accident and can appear in legitimate code that tries to call + // a generic function which emulates dynamic typing within C. + some_generic_function(IntPP, sizeof(*IntPP)); + some_generic_function(ClassPP, sizeof(*ClassPP)); + // Using `...[0]` instead of the dereference operator is another common + // variant, which is also widespread in the idiomatic array-size calculation: + // `sizeof(array) / sizeof(array[0])`. + some_generic_function(IntPP, sizeof(IntPP[0])); + some_generic_function(ClassPP, sizeof(ClassPP[0])); + // FIXME: There is a third common pattern where the generic function is + // called with `&Variable` and `sizeof(Variable)`. Right now these are + // reported by the `sizeof(pointer)` checks, but this causes some false + // positives, so it would be good to create an exception for them. + // NOTE: `sizeof(IntP)` is only reported with `WarnOnSizeOfPointer=true`. + some_generic_function(&IntPP, sizeof(IntP)); + some_generic_function(&ClassPP, sizeof(ClassP)); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer +} + int ValidExpressions() { int A[] = {1, 2, 3, 4}; static const char str[] = "hello";