diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp index a30e63f9b0fd6a..628d30ce7f73fe 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp @@ -70,7 +70,9 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name, Options.get("WarnOnSizeOfCompareToConstant", true)), WarnOnSizeOfPointerToAggregate( Options.get("WarnOnSizeOfPointerToAggregate", true)), - WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {} + WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)), + WarnOnOffsetDividedBySizeOf( + Options.get("WarnOnOffsetDividedBySizeOf", true)) {} void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant); @@ -82,6 +84,8 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnSizeOfPointerToAggregate", WarnOnSizeOfPointerToAggregate); Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer); + Options.store(Opts, "WarnOnOffsetDividedBySizeOf", + WarnOnOffsetDividedBySizeOf); } void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { @@ -307,7 +311,8 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { offsetOfExpr())) .bind("sizeof-in-ptr-arithmetic-scale-expr"); const auto PtrArithmeticIntegerScaleExpr = binaryOperator( - hasAnyOperatorName("*", "/"), + WarnOnOffsetDividedBySizeOf ? binaryOperator(hasAnyOperatorName("*", "/")) + : binaryOperator(hasOperatorName("*")), // sizeof(...) * sizeof(...) and sizeof(...) / sizeof(...) is handled // by this check on another path. hasOperands(expr(hasType(isInteger()), unless(SizeofLikeScaleExpr)), diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h index 66d7c34cc9e940..fbd62cb80fb2d0 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h @@ -31,6 +31,7 @@ class SizeofExpressionCheck : public ClangTidyCheck { const bool WarnOnSizeOfCompareToConstant; const bool WarnOnSizeOfPointerToAggregate; const bool WarnOnSizeOfPointer; + const bool WarnOnOffsetDividedBySizeOf; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b4792d749a86c3..57b0d2ab243487 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -144,7 +144,7 @@ Changes in existing checks - Improved :doc:`bugprone-sizeof-expression ` check to find suspicious usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or - subtracting from a pointer. + subtracting from a pointer directly or when used to scale a numeric value. - Improved :doc:`bugprone-unchecked-optional-access ` to support 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 89c88d2740dba2..29edb26ed7aa20 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 @@ -221,6 +221,17 @@ and the result is typically unintended, often out of bounds. ``Ptr + sizeof(T)`` will offset the pointer by ``sizeof(T)`` elements, effectively exponentiating the scaling factor to the power of 2. +Similarly, multiplying or dividing a numeric value with the ``sizeof`` of an +element or the whole buffer is suspicious, because the dimensional connection +between the numeric value and the actual ``sizeof`` result can not always be +deduced. +While scaling an integer up (multiplying) with ``sizeof`` is likely **always** +an issue, a scaling down (division) is not always inherently dangerous, in case +the developer is aware that the division happens between an appropriate number +of _bytes_ and a ``sizeof`` value. +Turning :option:`WarnOnOffsetDividedBySizeOf` off will restrict the +warnings to the multiplication case. + This case also checks suspicious ``alignof`` and ``offsetof`` usages in pointer arithmetic, as both return the "size" in bytes and not elements, potentially resulting in doubly-scaled offsets. @@ -299,3 +310,9 @@ Options ``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`. + +.. option:: WarnOnOffsetDividedBySizeOf + + When `true`, the check will warn on pointer arithmetic where the + element count is obtained from a division with ``sizeof(...)``, + e.g., ``Ptr + Bytes / sizeof(*T)``. Default is `true`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c new file mode 100644 index 00000000000000..b0cfb8d1cfa456 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c @@ -0,0 +1,14 @@ +// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- \ +// RUN: -config='{CheckOptions: { \ +// RUN: bugprone-sizeof-expression.WarnOnOffsetDividedBySizeOf: false \ +// RUN: }}' + +typedef __SIZE_TYPE__ size_t; + +void situational14(int *Buffer, size_t BufferSize) { + int *P = &Buffer[0]; + while (P < Buffer + BufferSize / sizeof(*Buffer)) { + // NO-WARNING: This test opted out of "P +- N */ sizeof(...)" warnings. + ++P; + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c index 360ce00a6889d7..712141c5f266aa 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c @@ -352,21 +352,30 @@ void good13(void) { int Buffer[BufferSize]; int *P = &Buffer[0]; - while (P < (Buffer + sizeof(Buffer) / sizeof(int))) { + while (P < Buffer + sizeof(Buffer) / sizeof(int)) { // NO-WARNING: Calculating the element count of the buffer here, which is // safe with this idiom (as long as the types don't change). ++P; } - while (P < (Buffer + sizeof(Buffer) / sizeof(Buffer[0]))) { + while (P < Buffer + sizeof(Buffer) / sizeof(Buffer[0])) { // NO-WARNING: Calculating the element count of the buffer here, which is // safe with this idiom. ++P; } - while (P < (Buffer + sizeof(Buffer) / sizeof(*P))) { + while (P < Buffer + sizeof(Buffer) / sizeof(*P)) { // NO-WARNING: Calculating the element count of the buffer here, which is // safe with this idiom. ++P; } } + +void situational14(int *Buffer, size_t BufferSize) { + int *P = &Buffer[0]; + while (P < Buffer + BufferSize / sizeof(*Buffer)) { + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic; this scaled value will be scaled again by the '+' operator + // CHECK-MESSAGES: :[[@LINE-2]]:21: note: '+' in pointer arithmetic internally scales with 'sizeof(int)' == {{[0-9]+}} + ++P; + } +}