https://github.com/danakj created https://github.com/llvm/llvm-project/pull/91991
This is built on top of https://github.com/llvm/llvm-project/pull/91777, and includes the commits there for now. >From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001 From: danakj <dan...@chromium.org> Date: Fri, 10 May 2024 13:31:17 -0400 Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors The -Wunsafe-buffer-usage warning should fire on any call to a function annotated with [[clang::unsafe_buffer_usage]], however it omitted calls to constructors, since the expression is a CXXConstructExpr which does not subclass CallExpr. Thus the matcher on callExpr() does not find these expressions. Add a new WarningGadget that matches cxxConstructExpr that are calling a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching against the std::span(ptr, size) constructor because that is handled by SpanTwoParamConstructorGadget and we never want two gadgets to match the same thing (and this is guarded by asserts). The gadgets themselves do not report the warnings, instead each gadget's Stmt is passed to the UnsafeBufferUsageHandler (implemented by UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a CXXConstructExpr statement must be a match for std::span(ptr, size), but that is no longer the case. We want the Reporter to generate different warnings (in the -Wunsafe-buffer-usage-in-container subgroup) for the span contructor. And we will want it to report more warnings for other std-container-specific gadgets in the future. To handle this we allow the gadget to control if the warning is general (it calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it calls handleUnsafeOperationInContainer()). Then the WarningGadget grows a virtual method to dispatch to the appropriate path in the UnsafeBufferUsageHandler. By doing so, we no longer need getBaseStmt in the Gadget interface. The only use of it for FixableGadgets was to get the SourceLocation, so we make an explicit virtual method for that on Gadget. Then the handleUnsafeOperation() dispatcher can be a virtual method that is only in WarningGadget. The SpanTwoParamConstructorGadget gadget dispatches to handleUnsafeOperationInContainer() while the other WarningGadgets all dispatch to the original handleUnsafeBufferUsage(). Tests are added for annotated constructors, conversion operattors, call operators, fold expressions, and regular methods. --- .../Analysis/Analyses/UnsafeBufferUsage.h | 5 + .../Analyses/UnsafeBufferUsageGadgets.def | 1 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 ++++++++++++------ clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++- ...warn-unsafe-buffer-usage-function-attr.cpp | 38 ++++ 5 files changed, 176 insertions(+), 57 deletions(-) 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..b24588e722aaa 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}; @@ -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 @@ -3008,8 +3068,9 @@ void clang::checkUnsafeBufferUsage(const Decl *D, // 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()); + llvm::errs() << "Warnings not EmitSuggestions\n"; + G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false, + D->getASTContext()); } // This return guarantees that most of the machine doesn't run when @@ -3251,8 +3312,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 +3324,8 @@ 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()); } } } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 6992ba9ad9a75..b3dd4bc6aa8f5 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2256,11 +2256,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { Range = UO->getSubExpr()->getSourceRange(); MsgParam = 1; } - } else if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) { - S.Diag(CtorExpr->getLocation(), - diag::warn_unsafe_buffer_usage_in_container); } else { - if (isa<CallExpr>(Operation)) { + if (isa<CallExpr>(Operation) || isa<CXXConstructExpr>(Operation)) { // note_unsafe_buffer_operation doesn't have this mode yet. assert(!IsRelatedToDecl && "Not implemented yet!"); MsgParam = 3; @@ -2295,6 +2292,23 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { } } + void handleUnsafeOperationInContainer(const Stmt *Operation, + bool IsRelatedToDecl, + ASTContext &Ctx) override { + SourceLocation Loc; + SourceRange Range; + unsigned MsgParam = 0; + if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) { + Loc = CtorExpr->getLocation(); + } + S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container); + if (IsRelatedToDecl) { + assert(!SuggestSuggestions && + "Variables blamed for unsafe buffer usage without suggestions!"); + S.Diag(Loc, diag::note_unsafe_buffer_operation) << MsgParam << Range; + } + } + void handleUnsafeVariableGroup(const VarDecl *Variable, const VariableGroupsManager &VarGrpMgr, FixItList &&Fixes, const Decl *D, diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp index 7df01c46438c7..bfc34b55c1f66 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp @@ -85,3 +85,41 @@ void testInheritance() { BC->func(); // expected-warning{{function introduces unsafe buffer manipulation}} BC->func1(); } + +class UnsafeMembers { +public: + UnsafeMembers() {} + + [[clang::unsafe_buffer_usage]] + UnsafeMembers(int) {} + + [[clang::unsafe_buffer_usage]] + explicit operator int() { return 0; } + + [[clang::unsafe_buffer_usage]] + void Method() {} + + [[clang::unsafe_buffer_usage]] + void operator()() {} + + [[clang::unsafe_buffer_usage]] + int operator+(UnsafeMembers) { return 0; } +}; + +template <class... Vs> +int testFoldExpression(Vs&&... v) { + return (... + v); // expected-warning{{function introduces unsafe buffer manipulation}} +} + +// https://github.com/llvm/llvm-project/issues/80482 +void testClassMembers() { + UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} + + (void)static_cast<int>(UnsafeMembers()); // expected-warning{{function introduces unsafe buffer manipulation}} + + UnsafeMembers().Method(); // expected-warning{{function introduces unsafe buffer manipulation}} + + UnsafeMembers()(); // expected-warning{{function introduces unsafe buffer manipulation}} + + testFoldExpression(UnsafeMembers(), UnsafeMembers()); +} >From 84da0fe9aa5d80c41ef78e79f6d47730f5be7880 Mon Sep 17 00:00:00 2001 From: danakj <dan...@chromium.org> Date: Fri, 10 May 2024 13:53:23 -0400 Subject: [PATCH 2/3] Remove debug logging --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index b24588e722aaa..9e07b9cbb6aea 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -3068,7 +3068,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D, // 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) { - llvm::errs() << "Warnings not EmitSuggestions\n"; G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false, D->getASTContext()); } >From b5a6a7de47741b2e6390acaa27f3f069bf5e1f8d Mon Sep 17 00:00:00 2001 From: danakj <dan...@chromium.org> Date: Mon, 13 May 2024 12:11:41 -0400 Subject: [PATCH 3/3] Generate -Wunsafe-buffer-usage warnings in ctor and field initializers Field initializers must be found by visiting FieldDecl and then running the warning checks/matchers against the in-class initializer, which is itself a Stmt. This catches warnings in places such as: struct C { P* Ptr; AnUnsafeCtor U{Ptr}; }; CXXCtorInitializers are not statements either, but they point to an initializer expression which is. When visiting a FunctionDecl, also walk through any constructor initializers and run the warning checks/matchers against their initializer expressions. This catches warnings for initializing fields and calling other constructors, such as: struct C { C(P* Ptr) : AnUnsafeCtor(Ptr) {} } We add tests for explicit construction, for field initialization, base class constructor calls, delegated constructor calls, and aggregate initialization. Note that aggregate initialization through `()` is treated differently today by the AST matchers than `{}`. The former is not considered as calling an implicit constructor, while the latter is. MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a TODO, which means we do not catch warnings of the form: struct AggregateInitType { AnUnsafeCtor U; } AggregateInitType{Ptr}; But we do already catch them when written as (in C++20): struct AggregateInitType { AnUnsafeCtor U; } AggregateInitType(Ptr); Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(), however, breaks expectations for field in-class initializers by moving the SourceLocation, possibly to inside the implicit ctor instead of on the line where the field initialization happens. struct C { P* Ptr; AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}} }; --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 ++++++++++++------ clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++ ...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++++++++++ 3 files changed, 145 insertions(+), 38 deletions(-) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 9e07b9cbb6aea..c7b4fcdce2132 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1387,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; @@ -1483,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)}; } @@ -3030,39 +3030,9 @@ 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 @@ -3106,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; @@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } } + +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 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; + } + } + + Stmts.push_back(FD->getBody()); + + if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) { + for (const CXXCtorInitializer* CI: ID->inits()) { + Stmts.push_back(CI->getInit()); + } + } + } + + if (const auto* FD = dyn_cast<FieldDecl>(D)) { + // Visit in-class initializers for fields. + if (!FD->hasInClassInitializer()) + return; + + Stmts.push_back(FD->getInClassInitializer()); + } + + if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) { + Stmts.push_back(D->getBody()); + } + + assert(!Stmts.empty()); + + for (Stmt *S : Stmts) { + auto [FixableGadgets, WarningGadgets, Tracker] = + findGadgets(S, D->getASTContext(), Handler, EmitSuggestions); + applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets), + std::move(Tracker), Handler, EmitSuggestions); + } +} diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index b3dd4bc6aa8f5..9aedae6a71b59 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2468,6 +2468,14 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> { CallableVisitor(llvm::function_ref<void(const Decl *)> Callback) : Callback(Callback) {} + bool VisitFieldDecl(FieldDecl *Node) { + if (cast<DeclContext>(Node->getParent())->isDependentContext()) + return true; // Not to analyze dependent decl + if (Node->hasInClassInitializer()) + Callback(Node); + return true; + } + bool VisitFunctionDecl(FunctionDecl *Node) { if (cast<DeclContext>(Node)->isDependentContext()) return true; // Not to analyze dependent decl diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp index bfc34b55c1f66..a0efecefe1c89 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp @@ -111,6 +111,42 @@ int testFoldExpression(Vs&&... v) { return (... + v); // expected-warning{{function introduces unsafe buffer manipulation}} } +struct HoldsUnsafeMembers { + HoldsUnsafeMembers() + : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}} + FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}} + {} + + [[clang::unsafe_buffer_usage]] + HoldsUnsafeMembers(int i) + : FromCtor(i), // expected-warning{{function introduces unsafe buffer manipulation}} + FromCtor2{i} // expected-warning{{function introduces unsafe buffer manipulation}} + {} + + HoldsUnsafeMembers(float f) + : HoldsUnsafeMembers(0) {} // expected-warning{{function introduces unsafe buffer manipulation}} + + UnsafeMembers FromCtor; + UnsafeMembers FromCtor2; + UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}} +}; + +struct SubclassUnsafeMembers : public UnsafeMembers { + SubclassUnsafeMembers() + : UnsafeMembers(3) // expected-warning{{function introduces unsafe buffer manipulation}} + {} + + [[clang::unsafe_buffer_usage]] + SubclassUnsafeMembers(int i) + : UnsafeMembers(i) // expected-warning{{function introduces unsafe buffer manipulation}} + {} +}; + +struct AggregateUnsafeMembers { + UnsafeMembers f1; + UnsafeMembers f2{3}; // expected-warning{{function introduces unsafe buffer manipulation}} +}; + // https://github.com/llvm/llvm-project/issues/80482 void testClassMembers() { UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} @@ -122,4 +158,35 @@ void testClassMembers() { UnsafeMembers()(); // expected-warning{{function introduces unsafe buffer manipulation}} testFoldExpression(UnsafeMembers(), UnsafeMembers()); + + HoldsUnsafeMembers(); + HoldsUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} + + SubclassUnsafeMembers(); + SubclassUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} + + (void)AggregateUnsafeMembers{ + .f1 = UnsafeMembers(3), // expected-warning{{function introduces unsafe buffer manipulation}} + }; + + (void)AggregateUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} + + // FIXME: This should generate a warning but InitListExpr construction is + // treated as calling an implicit constructor, while ParentListInitExpr (the + // one above) is not. So `MatchDescendantVisitor::shouldVisitImplicitCode()` + // must return true for this to generate a warning. However that moves the + // SourceLocation of warnings from field initializers that construct through + // InitListExpr, preventing us from matching warnings against them. + (void)AggregateUnsafeMembers{3}; } + +struct NotCalledHoldsUnsafeMembers { + NotCalledHoldsUnsafeMembers() + : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}} + FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}} + {} + + UnsafeMembers FromCtor; + UnsafeMembers FromCtor2; + UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}} +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
