llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Max (mxms0) <details> <summary>Changes</summary> This PR adds support for toggling on/off warnings around static sized arrays. This supports / addresses https://github.com/llvm/llvm-project/issues/87284, for those who use -fsanitize=array-bounds which inserts checks for fixed sized arrays already. --- Full diff: https://github.com/llvm/llvm-project/pull/176466.diff 7 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+5) - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+1-1) - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2-1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+17-2) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+6) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp (+159) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index ea41eb3becfcf..462dabcc41da7 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -178,6 +178,11 @@ class UnsafeBufferUsageHandler { virtual bool ignoreUnsafeBufferInLibcCall(const SourceLocation &Loc) const = 0; + /// \return true iff array subscript accesses on fixed size arrays should NOT + /// be reported at `Loc` + virtual bool + ignoreUnsafeBufferInStaticSizedArray(const SourceLocation &Loc) const = 0; + virtual std::string getUnsafeBufferUsageAttributeTextAt(SourceLocation Loc, StringRef WSSuffix = "") const = 0; diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index f9bba5d54e9c7..129ce95c1c0e0 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -33,12 +33,12 @@ WARNING_GADGET(Increment) WARNING_GADGET(Decrement) -WARNING_GADGET(ArraySubscript) WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) WARNING_GADGET(UnsafeBufferUsageCtorAttr) WARNING_GADGET(DataInvocation) WARNING_GADGET(UniquePtrArrayAccess) +WARNING_OPTIONAL_GADGET(ArraySubscript) WARNING_OPTIONAL_GADGET(UnsafeLibcFunctionCall) WARNING_OPTIONAL_GADGET(UnsafeFormatAttributedFunctionCall) WARNING_OPTIONAL_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)` diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 3764475fbd3df..f4717505e1bdd 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1774,7 +1774,8 @@ def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">; def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container">; def UnsafeBufferUsageInLibcCall : DiagGroup<"unsafe-buffer-usage-in-libc-call">; def UnsafeBufferUsageInUniquePtrArrayAccess : DiagGroup<"unsafe-buffer-usage-in-unique-ptr-array-access">; -def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer, UnsafeBufferUsageInLibcCall, UnsafeBufferUsageInUniquePtrArrayAccess]>; +def UnsafeBufferUsageInStaticSizedArray : DiagGroup<"unsafe-buffer-usage-in-static-sized-array">; +def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer, UnsafeBufferUsageInLibcCall, UnsafeBufferUsageInUniquePtrArrayAccess, UnsafeBufferUsageInStaticSizedArray]>; // Warnings and notes InstallAPI verification. def InstallAPIViolation : DiagGroup<"installapi-violation">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index eb7a608f798b8..1ab3f537d36a3 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -13472,6 +13472,8 @@ def warn_unsafe_buffer_usage_in_container : Warning< InGroup<UnsafeBufferUsageInContainer>, DefaultIgnore; def warn_unsafe_buffer_usage_unique_ptr_array_access : Warning<"direct access using operator[] on std::unique_ptr<T[]> is unsafe due to lack of bounds checking">, InGroup<UnsafeBufferUsageInUniquePtrArrayAccess>, DefaultIgnore; +def warn_unsafe_buffer_usage_in_static_sized_array : Warning<"direct access on T[N] is unsafe due to the lack of bounds checking">, + InGroup<UnsafeBufferUsageInStaticSizedArray>, DefaultIgnore; #ifndef NDEBUG // Not a user-facing diagnostic. Useful for debugging false negatives in // -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits). diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 6bb08102c0345..d7df7d8d46b61 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -667,7 +667,8 @@ static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node, } static bool isSafeArraySubscript(const ArraySubscriptExpr &Node, - const ASTContext &Ctx) { + const ASTContext &Ctx, + const bool IgnoreStaticSizedArrays) { // FIXME: Proper solution: // - refactor Sema::CheckArrayAccess // - split safe/OOB/unknown decision logic from diagnostics emitting code @@ -689,6 +690,12 @@ static bool isSafeArraySubscript(const ArraySubscriptExpr &Node, return false; } + if (IgnoreStaticSizedArrays) { + // If we made it here, it means a size was found for the var being + // accessed. If it's fixed size, we can ignore it. + return true; + } + Expr::EvalResult EVResult; const Expr *IndexExpr = Node.getIdx(); if (!IndexExpr->isValueDependent() && @@ -1568,6 +1575,7 @@ class ArraySubscriptGadget : public WarningGadget { } static bool matches(const Stmt *S, const ASTContext &Ctx, + const UnsafeBufferUsageHandler *Handler, MatchResult &Result) { const auto *ASE = dyn_cast<ArraySubscriptExpr>(S); if (!ASE) @@ -1578,7 +1586,10 @@ class ArraySubscriptGadget : public WarningGadget { const auto *Idx = dyn_cast<IntegerLiteral>(ASE->getIdx()); bool IsSafeIndex = (Idx && Idx->getValue().isZero()) || isa<ArrayInitIndexExpr>(ASE->getIdx()); - if (IsSafeIndex || isSafeArraySubscript(*ASE, Ctx)) + if (IsSafeIndex || + isSafeArraySubscript( + *ASE, Ctx, + Handler->ignoreUnsafeBufferInStaticSizedArray(S->getBeginLoc()))) return false; Result.addNode(ArraySubscrTag, DynTypedNode::create(*ASE)); return true; @@ -2888,6 +2899,10 @@ std::set<const Expr *> clang::findUnsafePointers(const FunctionDecl *FD) { bool ignoreUnsafeBufferInLibcCall(const SourceLocation &) const override { return false; } + bool ignoreUnsafeBufferInStaticSizedArray( + const SourceLocation &Loc) const override { + return false; + } std::string getUnsafeBufferUsageAttributeTextAt( SourceLocation, StringRef WSSuffix = "") const override { return ""; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 56d7db649afbe..ab6fc49ef8233 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2629,6 +2629,12 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { return S.Diags.isIgnored(diag::warn_unsafe_buffer_libc_call, Loc); } + bool ignoreUnsafeBufferInStaticSizedArray( + const SourceLocation &Loc) const override { + return S.Diags.isIgnored( + diag::warn_unsafe_buffer_usage_in_static_sized_array, Loc); + } + // Returns the text representation of clang::unsafe_buffer_usage attribute. // `WSSuffix` holds customized "white-space"s, e.g., newline or whilespace // characters. diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp new file mode 100644 index 0000000000000..c4813198bbd9a --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-static-sized-array.cpp @@ -0,0 +1,159 @@ +// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \ +// RUN: -Wno-unsafe-buffer-usage-in-static-sized-array \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -verify %s + +// CHECK-NOT: [-Wunsafe-buffer-usage] +// expected-no-diagnostics + +void foo(unsigned idx) { + int buffer[10]; + buffer[idx] = 0; +} + +int global_buffer[10]; +void foo2(unsigned idx) { global_buffer[idx] = 0; } + +struct Foo { + int member_buffer[10]; + int x; +}; + +void foo2(Foo &f, unsigned idx) { f.member_buffer[idx] = 0; } + +void constant_idx_safe(unsigned idx) { + int buffer[10]; + buffer[9] = 0; +} + +void constant_idx_safe0(unsigned idx) { + int buffer[10]; + buffer[0] = 0; +} + +int array[10]; + +void circular_access_unsigned(unsigned idx) { + array[idx % 10]; + array[idx % 11]; + array[(idx + 3) % 10]; + array[(--idx) % 8]; + array[idx & 9 % 10]; + array[9 & idx % 11]; + array[12 % 10]; +} + +void circular_access_signed(int idx) { array[idx % 10]; } + +void masked_idx1(unsigned long long idx, Foo f) { + // Bitwise and operation + array[idx & 5] = 10; + array[5 & idx] = 12; + array[idx & 11 & 5] = 3; + array[idx & 11] = 20; + array[idx &= 5]; + array[f.x & 5]; + array[5 & f.x]; + array[f.x & (-5)]; +} + +typedef unsigned long long uint64_t; +typedef unsigned int uint32_t; +typedef unsigned char uint8_t; + +void type_conversions(uint64_t idx1, uint32_t idx2, uint8_t idx3) { + array[(uint32_t)idx1 & 3]; + array[idx2 & 3]; + array[idx3 & 3]; +} + +int array2[5]; + +void masked_idx_safe(unsigned long long idx) { + array2[6 & 5]; + array2[6 & idx & (idx + 1) & 5]; +} + +void constant_idx_unsafe(unsigned idx) { + int buffer[10]; + buffer[10] = 0; +} + +void constant_id_string(unsigned idx) { + char safe_char = "abc"[1]; + safe_char = ""[0]; + safe_char = "\0"[0]; + + char abcd[5] = "abc"; + abcd[2]; + + char unsafe_char = "abc"[3]; + unsafe_char = "abc"[-1]; + unsafe_char = ""[1]; + unsafe_char = ""[idx]; +} + +typedef float Float4x4[4][4]; + +float two_dimension_array(Float4x4 &matrix, unsigned idx) { + float a = matrix[0][4]; + + a = matrix[0][3]; + + a = matrix[4][0]; + + a = matrix[idx][0]; + + a = matrix[0][idx]; + + a = matrix[idx][idx]; + + return matrix[1][1]; +} + +typedef float Float2x3x4[2][3][4]; +float multi_dimension_array(Float2x3x4 &matrix) { + float *f = matrix[0][2]; + return matrix[1][2][3]; +} + +char array_strings[][11] = {"Apple", "Banana", "Cherry", "Date", "Elderberry"}; + +char array_string[] = "123456"; + +char access_strings() { + char c = array_strings[0][4]; + c = array_strings[3][10]; + c = array_string[5]; + return c; +} + +struct T { + int array[10]; +}; + +const int index = 1; + +constexpr int get_const(int x) { + if (x < 3) + return ++x; + else + return x + 5; +}; + +void array_indexed_const_expr(unsigned idx) { + int arr[10]; + arr[sizeof(int)] = 5; + + int array[sizeof(T)]; + array[sizeof(int)] = 5; + array[sizeof(T) - 1] = 3; + + int k = arr[6 & 5]; + k = arr[2 << index]; + k = arr[8 << index]; + k = arr[16 >> 1]; + k = arr[get_const(index)]; + k = arr[get_const(5)]; + k = arr[get_const(4)]; +} `````````` </details> https://github.com/llvm/llvm-project/pull/176466 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
