llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: None (jkorous-apple) <details> <summary>Changes</summary> depends on https://github.com/llvm/llvm-project/pull/80347 --- Full diff: https://github.com/llvm/llvm-project/pull/81343.diff 4 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+2-1) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+99-15) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp (+1-1) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp (+43) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index 07f805ebb11013..3273c642eed517 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -45,7 +45,8 @@ FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Poin FIXABLE_GADGET(UPCStandalonePointer) FIXABLE_GADGET(UPCPreIncrement) // '++Ptr' in an Unspecified Pointer Context FIXABLE_GADGET(UUCAddAssign) // 'Ptr += n' in an Unspecified Untyped Context -FIXABLE_GADGET(PointerAssignment) +FIXABLE_GADGET(PtrToPtrAssignment) +FIXABLE_GADGET(CArrayToPtrAssignment) FIXABLE_GADGET(PointerInit) #undef FIXABLE_GADGET diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index a74c113e29f1cf..8e810876950c81 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -7,11 +7,14 @@ //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" +#include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Stmt.h" #include "clang/AST/StmtVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Basic/CharInfo.h" #include "clang/Basic/SourceLocation.h" #include "clang/Lex/Lexer.h" @@ -799,7 +802,8 @@ class PointerInitGadget : public FixableGadget { /// \code /// p = q; /// \endcode -class PointerAssignmentGadget : public FixableGadget { +/// where both `p` and `q` are pointers. +class PtrToPtrAssignmentGadget : public FixableGadget { private: static constexpr const char *const PointerAssignLHSTag = "ptrLHS"; static constexpr const char *const PointerAssignRHSTag = "ptrRHS"; @@ -807,13 +811,13 @@ class PointerAssignmentGadget : public FixableGadget { const DeclRefExpr * PtrRHS; // the RHS pointer expression in `PA` public: - PointerAssignmentGadget(const MatchFinder::MatchResult &Result) - : FixableGadget(Kind::PointerAssignment), - PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)), - PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {} + PtrToPtrAssignmentGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::PtrToPtrAssignment), + PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)), + PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {} static bool classof(const Gadget *G) { - return G->getKind() == Kind::PointerAssignment; + return G->getKind() == Kind::PtrToPtrAssignment; } static Matcher matcher() { @@ -848,6 +852,60 @@ class PointerAssignmentGadget : public FixableGadget { } }; +/// An assignment expression of the form: +/// \code +/// ptr = array; +/// \endcode +/// where `p` is a pointer and `array` is a constant size array. +class CArrayToPtrAssignmentGadget : public FixableGadget { +private: + static constexpr const char *const PointerAssignLHSTag = "ptrLHS"; + static constexpr const char *const PointerAssignRHSTag = "ptrRHS"; + const DeclRefExpr *PtrLHS; // the LHS pointer expression in `PA` + const DeclRefExpr *PtrRHS; // the RHS pointer expression in `PA` + +public: + CArrayToPtrAssignmentGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::CArrayToPtrAssignment), + PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)), + PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {} + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::CArrayToPtrAssignment; + } + + static Matcher matcher() { + auto PtrAssignExpr = binaryOperator( + allOf(hasOperatorName("="), + hasRHS(ignoringParenImpCasts( + declRefExpr(hasType(hasCanonicalType(constantArrayType())), + toSupportedVariable()) + .bind(PointerAssignRHSTag))), + hasLHS(declRefExpr(hasPointerType(), toSupportedVariable()) + .bind(PointerAssignLHSTag)))); + + return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr)); + } + + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; + + virtual const Stmt *getBaseStmt() const override { + // FIXME: This should be the binary operator, assuming that this method + // makes sense at all on a FixableGadget. + return PtrLHS; + } + + virtual DeclUseList getClaimedVarUseSites() const override { + return DeclUseList{PtrLHS, PtrRHS}; + } + + virtual std::optional<std::pair<const VarDecl *, const VarDecl *>> + getStrategyImplications() const override { + return {}; + } +}; + /// A call of a function or method that performs unchecked buffer operations /// over one of its pointer parameters. class UnsafeBufferUsageAttrGadget : public WarningGadget { @@ -1471,7 +1529,7 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts, } std::optional<FixItList> -PointerAssignmentGadget::getFixits(const FixitStrategy &S) const { +PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl()); const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl()); switch (S.lookup(LeftVD)) { @@ -1490,6 +1548,26 @@ PointerAssignmentGadget::getFixits(const FixitStrategy &S) const { return std::nullopt; } +/// \returns fixit that adds .data() call after \DRE. +static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx, + const DeclRefExpr *DRE); + +std::optional<FixItList> +CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const { + const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl()); + const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl()); + if (S.lookup(LeftVD) == FixitStrategy::Kind::Span) { + if (S.lookup(RightVD) == FixitStrategy::Kind::Wontfix) { + return FixItList{}; + } + } else if (S.lookup(LeftVD) == FixitStrategy::Kind::Wontfix) { + if (S.lookup(RightVD) == FixitStrategy::Kind::Array) { + return createDataFixit(RightVD->getASTContext(), PtrRHS); + } + } + return std::nullopt; +} + std::optional<FixItList> PointerInitGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = PtrInitLHS; @@ -1907,6 +1985,19 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const { return std::nullopt; } +static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx, + const DeclRefExpr *DRE) { + const SourceManager &SM = Ctx.getSourceManager(); + // Inserts the .data() after the DRE + std::optional<SourceLocation> EndOfOperand = + getPastLoc(DRE, SM, Ctx.getLangOpts()); + + if (EndOfOperand) + return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}}; + + return std::nullopt; +} + // Generates fix-its replacing an expression of the form UPC(DRE) with // `DRE.data()` std::optional<FixItList> @@ -1915,14 +2006,7 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const { switch (S.lookup(VD)) { case FixitStrategy::Kind::Array: case FixitStrategy::Kind::Span: { - ASTContext &Ctx = VD->getASTContext(); - SourceManager &SM = Ctx.getSourceManager(); - // Inserts the .data() after the DRE - std::optional<SourceLocation> EndOfOperand = - getPastLoc(Node, SM, Ctx.getLangOpts()); - - if (EndOfOperand) - return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}}; + return createDataFixit(VD->getASTContext(), Node); // FIXME: Points inside a macro expansion. break; } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp index a5b578b98d4e5b..4cc1948f28a773 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp @@ -53,7 +53,7 @@ void unclaimed_use() { void implied_unclaimed_var(int *b) { // expected-warning{{'b' is an unsafe pointer used for buffer access}} int *a = new int[3]; // expected-warning{{'a' is an unsafe pointer used for buffer access}} a[4] = 7; // expected-note{{used in buffer access here}} - a = b; // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}} + a = b; // debug-note{{safe buffers debug: gadget 'PtrToPtrAssignment' refused to produce a fix}} b++; // expected-note{{used in pointer arithmetic here}} \ // debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}} } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp new file mode 100644 index 00000000000000..020a4520e2ccb4 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \ +// RUN: -fsafe-buffer-usage-suggestions \ +// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +void safe_array_assigned_to_safe_ptr(unsigned idx) { + int buffer[10]; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int* ptr; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + ptr = buffer; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: +} + +void safe_array_assigned_to_unsafe_ptr(unsigned idx) { + int buffer[10]; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int* ptr; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr" + ptr = buffer; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + ptr[idx] = 0; +} + +void unsafe_array_assigned_to_safe_ptr(unsigned idx) { + int buffer[10]; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer" + int* ptr; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + ptr = buffer; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()" + buffer[idx] = 0; +} + +void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) { + int buffer[10]; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}} + int* ptr; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}} + ptr = buffer; + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}} + buffer[idx] = 0; + ptr[idx] = 0; +} `````````` </details> https://github.com/llvm/llvm-project/pull/81343 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
