diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index f0c99a767071f..a52b00bf8d4ce 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -34,6 +34,7 @@ FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalu FIXABLE_GADGET(DerefSimplePtrArithFixable) FIXABLE_GADGET(PointerDereference) FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context +FIXABLE_GADGET(UPCStandalonePointer) #undef FIXABLE_GADGET #undef WARNING_GADGET diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 1ef276ab90444..9fb928f6c4c68 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -165,17 +165,22 @@ isInUnspecifiedPointerContext(internal::Matcher InnerMatcher) { // 1. an argument of a function call (except the callee has [[unsafe_...]] // attribute), or // 2. the operand of a cast operation; or - // ... + // 3. the operand of a comparator operation; or auto CallArgMatcher = - callExpr(hasAnyArgument(allOf( - hasPointerType() /* array also decays to pointer type*/, - InnerMatcher)), - unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))); + callExpr(forEachArgumentWithParam(InnerMatcher, + hasPointerType() /* array also decays to pointer type*/), + unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))); + auto CastOperandMatcher = explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral), castSubExpr(allOf(hasPointerType(), InnerMatcher))); - return stmt(anyOf(CallArgMatcher, CastOperandMatcher)); + auto CompOperandMatcher = + binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="), + eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)), + hasRHS(allOf(hasPointerType(), InnerMatcher)))); + + return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher)); // FIXME: any more cases? (UPC excludes the RHS of an assignment. For now we // don't have to check that.) } @@ -489,6 +494,41 @@ class ULCArraySubscriptGadget : public FixableGadget { } }; +// Fixable gadget to handle stand alone pointers of the form `UPC(DRE)` in the +// unspecified pointer context (isInUnspecifiedPointerContext). The gadget emits +// fixit of the form `UPC(DRE.data())`. +class UPCStandalonePointerGadget : public FixableGadget { +private: + static constexpr const char *const DeclRefExprTag = "StandalonePointer"; + const DeclRefExpr *Node; + +public: + UPCStandalonePointerGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::UPCStandalonePointer), + Node(Result.Nodes.getNodeAs(DeclRefExprTag)) { + assert(Node != nullptr && "Expecting a non-null matching result"); + } + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::UPCStandalonePointer; + } + + static Matcher matcher() { + auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType()); + auto target = expr( + ignoringParenImpCasts(declRefExpr(allOf(ArrayOrPtr, to(varDecl()))).bind(DeclRefExprTag))); + return stmt(isInUnspecifiedPointerContext(target)); + } + + virtual std::optional getFixits(const Strategy &S) const override; + + virtual const Stmt *getBaseStmt() const override { return Node; } + + virtual DeclUseList getClaimedVarUseSites() const override { + return {Node}; + } +}; + class PointerDereferenceGadget : public FixableGadget { static constexpr const char *const BaseDeclRefExprTag = "BaseDRE"; static constexpr const char *const OperatorTag = "op"; @@ -1070,6 +1110,31 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const { return std::nullopt; } +// Generates fix-its replacing an expression of the form UPC(DRE) with +// `DRE.data()` +std::optional UPCStandalonePointerGadget::getFixits(const Strategy &S) + const { + const auto VD = cast(Node->getDecl()); + switch (S.lookup(VD)) { + case Strategy::Kind::Span: { + ASTContext &Ctx = VD->getASTContext(); + SourceManager &SM = Ctx.getSourceManager(); + // Inserts the .data() after the DRE + SourceLocation endOfOperand = getEndCharLoc(Node, SM, Ctx.getLangOpts()); + + return FixItList{{FixItHint::CreateInsertion( + endOfOperand.getLocWithOffset(1), ".data()")}}; + } + case Strategy::Kind::Wontfix: + case Strategy::Kind::Iterator: + case Strategy::Kind::Array: + case Strategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); + } + + return std::nullopt; +} + // Generates fix-its replacing an expression of the form `&DRE[e]` with // `&DRE.data()[e]`: static std::optional diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp new file mode 100644 index 0000000000000..109f8b19db5aa --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp @@ -0,0 +1,110 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +void foo(int* v) { +} + +void m1(int* v1, int* v2, int) { + +} + +void condition_check_nullptr() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + if(p != nullptr) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" +} + +void condition_check() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + auto q = new int[10]; + + int tmp = p[5]; + if(p == q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(q != p){} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:12}:".data()" + + if(p < q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p <= q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p > q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p >= q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if( p == q && p != nullptr) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:".data()" +} + +void condition_check_two_usafe_buffers() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + auto q = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span q" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + tmp = q[5]; + + if(p == q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:".data()" +} + +void unsafe_method_invocation_single_param() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + foo(p); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()" + +} + +void safe_method_invocation_single_param() { + auto p = new int[10]; + foo(p); +} + +void unsafe_method_invocation_double_param() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + m1(p, p, 10); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:".data()" + + auto q = new int[10]; + + m1(p, q, 4); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + m1(q, p, 9); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()" + + m1(q, q, 8); +} +