Author: whisperity Date: 2024-10-15T14:42:57+02:00 New Revision: 1c38c46b083315e3a621267c9a90e8a7750f3700
URL: https://github.com/llvm/llvm-project/commit/1c38c46b083315e3a621267c9a90e8a7750f3700 DIFF: https://github.com/llvm/llvm-project/commit/1c38c46b083315e3a621267c9a90e8a7750f3700.diff LOG: [clang-tidy] Make `P +- BS / sizeof(*P)` opt-outable in `bugprone-sizeof-expression` (#111178) In some cases and for projects that deal with a lot of low-level buffers, a pattern often emerges that an array and its full size, not in the number of "elements" but in "bytes", are known with no syntax-level connection between the two values. To access the array elements, the pointer arithmetic involved will have to divide 'SizeInBytes' (a numeric value) with `sizeof(*Buffer)`. Since the previous patch introduced this new warning, potential false-positives were triggered from `bugprone-sizeof-expression`, as `sizeof` appeared in pointer arithmetic where integers are scaled. This patch adds a new check option, `WarnOnOffsetDividedBySizeOf`, which allows users to opt out of warning about the division case. In arbitrary projects, it might still be worthwhile to get these warnings until an opt-out from the detection of scaling issues, especially if a project might not be using low-level buffers intensively. Added: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics-no-division.c Modified: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-pointer-arithmetics.c Removed: ################################################################################ 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 6196b9e15d3fc5..95be0a89cd6c93 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -158,7 +158,7 @@ Changes in existing checks - Improved :doc:`bugprone-sizeof-expression <clang-tidy/checks/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 <clang-tidy/checks/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; + } +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
