llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Dana Jansens (danakj) <details> <summary>Changes</summary> This is built on top of https://github.com/llvm/llvm-project/pull/91777, and includes the commits there for now. --- Patch is 28.06 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91991.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+5) - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+1) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+183-91) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+26-4) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp (+105) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 5d16dcc824c50..228b4ae1e3e11 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler { virtual void handleUnsafeOperation(const Stmt *Operation, bool IsRelatedToDecl, ASTContext &Ctx) = 0; + /// Invoked when an unsafe operation with a std container is found. + virtual void handleUnsafeOperationInContainer(const Stmt *Operation, + bool IsRelatedToDecl, + ASTContext &Ctx) = 0; + /// Invoked when a fix is suggested against a variable. This function groups /// all variables that must be fixed together (i.e their types must be changed /// to the same target type to prevent type mismatches) into a single fixit. diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index 3273c642eed51..242ad763ba62b 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -36,6 +36,7 @@ WARNING_GADGET(Decrement) WARNING_GADGET(ArraySubscript) WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) +WARNING_GADGET(UnsafeBufferUsageCtorAttr) WARNING_GADGET(DataInvocation) WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)` FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index c42e70d5b95ac..c7b4fcdce2132 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -492,7 +492,7 @@ class Gadget { #endif virtual bool isWarningGadget() const = 0; - virtual const Stmt *getBaseStmt() const = 0; + virtual SourceLocation getSourceLoc() const = 0; /// Returns the list of pointer-type variables on which this gadget performs /// its operation. Typically, there's only one variable. This isn't a list @@ -513,6 +513,10 @@ class WarningGadget : public Gadget { static bool classof(const Gadget *G) { return G->isWarningGadget(); } bool isWarningGadget() const final { return true; } + + virtual void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const = 0; }; /// Fixable gadgets correspond to code patterns that aren't always unsafe but @@ -572,7 +576,12 @@ class IncrementGadget : public WarningGadget { .bind(OpTag)); } - const UnaryOperator *getBaseStmt() const override { return Op; } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { SmallVector<const DeclRefExpr *, 2> Uses; @@ -607,7 +616,12 @@ class DecrementGadget : public WarningGadget { .bind(OpTag)); } - const UnaryOperator *getBaseStmt() const override { return Op; } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { if (const auto *DRE = @@ -648,7 +662,12 @@ class ArraySubscriptGadget : public WarningGadget { // clang-format on } - const ArraySubscriptExpr *getBaseStmt() const override { return ASE; } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(ASE, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return ASE->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { if (const auto *DRE = @@ -696,7 +715,12 @@ class PointerArithmeticGadget : public WarningGadget { .bind(PointerArithmeticTag)); } - const Stmt *getBaseStmt() const override { return PA; } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(PA, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return PA->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { if (const auto *DRE = dyn_cast<DeclRefExpr>(Ptr->IgnoreParenImpCasts())) { @@ -734,7 +758,12 @@ class SpanTwoParamConstructorGadget : public WarningGadget { .bind(SpanTwoParamConstructorTag)); } - const Stmt *getBaseStmt() const override { return Ctor; } + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperationInContainer(Ctor, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return Ctor->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { // If the constructor call is of the form `std::span{var, n}`, `var` is @@ -780,11 +809,8 @@ class PointerInitGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; - - virtual const Stmt *getBaseStmt() const override { - // FIXME: This needs to be the entire DeclStmt, assuming that this method - // makes sense at all on a FixableGadget. - return PtrInitRHS; + SourceLocation getSourceLoc() const override { + return PtrInitRHS->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { @@ -833,12 +859,7 @@ class PtrToPtrAssignmentGadget : public FixableGadget { 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; - } + SourceLocation getSourceLoc() const override { return PtrLHS->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { return DeclUseList{PtrLHS, PtrRHS}; @@ -888,12 +909,7 @@ class CArrayToPtrAssignmentGadget : public FixableGadget { 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; - } + SourceLocation getSourceLoc() const override { return PtrLHS->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { return DeclUseList{PtrLHS, PtrRHS}; @@ -921,10 +937,55 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget { } static Matcher matcher() { - return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))) - .bind(OpTag)); + auto HasUnsafeFnDecl = + callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))); + return stmt(callExpr(HasUnsafeFnDecl).bind(OpTag)); + } + + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } + + DeclUseList getClaimedVarUseSites() const override { return {}; } +}; + +/// A call of a constructor that performs unchecked buffer operations +/// over one of its pointer parameters, or constructs a class object that will +/// perform buffer operations that depend on the correctness of the parameters. +class UnsafeBufferUsageCtorAttrGadget : public WarningGadget { + constexpr static const char *const OpTag = "cxx_construct_expr"; + const CXXConstructExpr *Op; + +public: + UnsafeBufferUsageCtorAttrGadget(const MatchFinder::MatchResult &Result) + : WarningGadget(Kind::UnsafeBufferUsageCtorAttr), + Op(Result.Nodes.getNodeAs<CXXConstructExpr>(OpTag)) {} + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::UnsafeBufferUsageCtorAttr; + } + + static Matcher matcher() { + auto HasUnsafeCtorDecl = + hasDeclaration(cxxConstructorDecl(hasAttr(attr::UnsafeBufferUsage))); + // std::span(ptr, size) ctor is handled by SpanTwoParamConstructorGadget. + auto HasTwoParamSpanCtorDecl = hasDeclaration( + cxxConstructorDecl(hasDeclContext(isInStdNamespace()), hasName("span"), + parameterCountIs(2))); + return stmt( + cxxConstructExpr(HasUnsafeCtorDecl, unless(HasTwoParamSpanCtorDecl)) + .bind(OpTag)); + } + + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx); } - const Stmt *getBaseStmt() const override { return Op; } + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { return {}; } }; @@ -953,7 +1014,13 @@ class DataInvocationGadget : public WarningGadget { explicitCastExpr(anyOf(has(callExpr), has(parenExpr(has(callExpr))))) .bind(OpTag)); } - const Stmt *getBaseStmt() const override { return Op; } + + void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler, + bool IsRelatedToDecl, + ASTContext &Ctx) const override { + Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx); + } + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } DeclUseList getClaimedVarUseSites() const override { return {}; } }; @@ -990,8 +1057,7 @@ class ULCArraySubscriptGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; - - virtual const Stmt *getBaseStmt() const override { return Node; } + SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { if (const auto *DRE = @@ -1031,8 +1097,7 @@ class UPCStandalonePointerGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; - - virtual const Stmt *getBaseStmt() const override { return Node; } + SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { return {Node}; } }; @@ -1070,10 +1135,9 @@ class PointerDereferenceGadget : public FixableGadget { return {BaseDeclRefExpr}; } - virtual const Stmt *getBaseStmt() const final { return Op; } - virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; + SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); } }; // Represents expressions of the form `&DRE[any]` in the Unspecified Pointer @@ -1108,8 +1172,7 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &) const override; - - virtual const Stmt *getBaseStmt() const override { return Node; } + SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { const auto *ArraySubst = cast<ArraySubscriptExpr>(Node->getSubExpr()); @@ -1218,8 +1281,7 @@ class UPCPreIncrementGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; - - virtual const Stmt *getBaseStmt() const override { return Node; } + SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { return {dyn_cast<DeclRefExpr>(Node->getSubExpr())}; @@ -1264,8 +1326,7 @@ class UUCAddAssignGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &S) const override; - - virtual const Stmt *getBaseStmt() const override { return Node; } + SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); } virtual DeclUseList getClaimedVarUseSites() const override { return {dyn_cast<DeclRefExpr>(Node->getLHS())}; @@ -1315,9 +1376,9 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget { virtual std::optional<FixItList> getFixits(const FixitStrategy &s) const final; - - // TODO remove this method from FixableGadget interface - virtual const Stmt *getBaseStmt() const final { return nullptr; } + SourceLocation getSourceLoc() const override { + return DerefOp->getBeginLoc(); + } virtual DeclUseList getClaimedVarUseSites() const final { return {BaseDeclRefExpr}; @@ -1326,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget { /// Scan the function and return a list of gadgets found with provided kits. static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker> -findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler, - bool EmitSuggestions) { +findGadgets(const Stmt *S, ASTContext &Ctx, + const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) { struct GadgetFinderCallback : MatchFinder::MatchCallback { FixableGadgetList FixableGadgets; @@ -1422,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler, // clang-format on } - M.match(*D->getBody(), D->getASTContext()); + M.match(*S, Ctx); return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; } @@ -2070,7 +2131,7 @@ UUCAddAssignGadget::getFixits(const FixitStrategy &S) const { if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; - const Stmt *AddAssignNode = getBaseStmt(); + const Stmt *AddAssignNode = Node; StringRef varName = VD->getName(); const ASTContext &Ctx = VD->getASTContext(); @@ -2112,7 +2173,6 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const { if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; std::stringstream SS; - const Stmt *PreIncNode = getBaseStmt(); StringRef varName = VD->getName(); const ASTContext &Ctx = VD->getASTContext(); @@ -2120,12 +2180,12 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const { SS << "(" << varName.data() << " = " << varName.data() << ".subspan(1)).data()"; std::optional<SourceLocation> PreIncLocation = - getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts()); + getEndCharLoc(Node, Ctx.getSourceManager(), Ctx.getLangOpts()); if (!PreIncLocation) return std::nullopt; Fixes.push_back(FixItHint::CreateReplacement( - SourceRange(PreIncNode->getBeginLoc(), *PreIncLocation), SS.str())); + SourceRange(Node->getBeginLoc(), *PreIncLocation), SS.str())); return Fixes; } } @@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, } #ifndef NDEBUG Handler.addDebugNoteForVar( - VD, F->getBaseStmt()->getBeginLoc(), + VD, F->getSourceLoc(), ("gadget '" + F->getDebugName() + "' refused to produce a fix") .str()); #endif @@ -2970,46 +3030,16 @@ class VariableGroupsManagerImpl : public VariableGroupsManager { } }; -void clang::checkUnsafeBufferUsage(const Decl *D, - UnsafeBufferUsageHandler &Handler, - bool EmitSuggestions) { -#ifndef NDEBUG - Handler.clearDebugNotes(); -#endif - - assert(D && D->getBody()); - // We do not want to visit a Lambda expression defined inside a method - // independently. Instead, it should be visited along with the outer method. - // FIXME: do we want to do the same thing for `BlockDecl`s? - if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) { - if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass()) - return; - } - - // Do not emit fixit suggestions for functions declared in an - // extern "C" block. - if (const auto *FD = dyn_cast<FunctionDecl>(D)) { - for (FunctionDecl *FReDecl : FD->redecls()) { - if (FReDecl->isExternC()) { - EmitSuggestions = false; - break; - } - } - } - - WarningGadgetSets UnsafeOps; - FixableGadgetSets FixablesForAllVars; - - auto [FixableGadgets, WarningGadgets, Tracker] = - findGadgets(D, Handler, EmitSuggestions); - +void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets, + WarningGadgetList WarningGadgets, DeclUseTracker Tracker, + UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) { if (!EmitSuggestions) { // Our job is very easy without suggestions. Just warn about // every problematic operation and consider it done. No need to deal // with fixable gadgets, no need to group operations by variable. for (const auto &G : WarningGadgets) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false, - D->getASTContext()); + G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false, + D->getASTContext()); } // This return guarantees that most of the machine doesn't run when @@ -3046,8 +3076,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D, if (WarningGadgets.empty()) return; - UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); - FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets)); + WarningGadgetSets UnsafeOps = + groupWarningGadgetsByVar(std::move(WarningGadgets)); + FixableGadgetSets FixablesForAllVars = + groupFixablesByVar(std::move(FixableGadgets)); std::map<const VarDecl *, FixItList> FixItsForVariableGroup; @@ -3251,8 +3283,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D, Tracker, Handler, VarGrpMgr); for (const auto &G : UnsafeOps.noVar) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false, - D->getASTContext()); + G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false, + D->getASTContext()); } for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { @@ -3263,8 +3295,68 @@ void clang::checkUnsafeBufferUsage(const Decl *D, : FixItList{}, D, NaiveStrategy); for (const auto &G : WarningGadgets) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true, - D->getASTContext()); + G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/true, + D->getASTContext()); + } + } +} + +void clang::checkUnsafeBufferUsage(const Decl *D, + UnsafeBufferUsageHandler &Handler, + bool EmitSuggestions) { +#ifndef NDEBUG + Handler.clearDebugNotes(); +#endif + + assert(D); + + SmallVector<Stmt*> Stmts; + + // We do not want to visit a Lambda expression defined inside a method + // independently. Instead, it should be visited along with the outer method. + // FIXME: do we want to do the same ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/91991 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
