Author: Zequan Wu Date: 2024-11-27T11:18:05-08:00 New Revision: c60b055d463a3e9f18a494aec075f35d38d447a0
URL: https://github.com/llvm/llvm-project/commit/c60b055d463a3e9f18a494aec075f35d38d447a0 DIFF: https://github.com/llvm/llvm-project/commit/c60b055d463a3e9f18a494aec075f35d38d447a0.diff LOG: Reapply "Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (#91991)" It was originally reverted due to an [existing bug](https://github.com/llvm/llvm-project/issues/116286). Relanding it as the bug was fixed by https://github.com/llvm/llvm-project/pull/116433. Added: Modified: clang/lib/Analysis/UnsafeBufferUsage.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 321097e16a45f7..40f529e52b44af 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -172,6 +172,12 @@ class MatchDescendantVisitor : public DynamicRecursiveASTVisitor { return DynamicRecursiveASTVisitor::TraverseCXXTypeidExpr(Node); } + bool TraverseCXXDefaultInitExpr(CXXDefaultInitExpr *Node) override { + if (!TraverseStmt(Node->getExpr())) + return false; + return DynamicRecursiveASTVisitor::TraverseCXXDefaultInitExpr(Node); + } + bool TraverseStmt(Stmt *Node) override { if (!Node) return true; @@ -1987,14 +1993,18 @@ 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) { +static void findGadgets(const Stmt *S, ASTContext &Ctx, + const UnsafeBufferUsageHandler &Handler, + bool EmitSuggestions, FixableGadgetList &FixableGadgets, + WarningGadgetList &WarningGadgets, + DeclUseTracker &Tracker) { struct GadgetFinderCallback : MatchFinder::MatchCallback { - FixableGadgetList FixableGadgets; - WarningGadgetList WarningGadgets; - DeclUseTracker Tracker; + GadgetFinderCallback(FixableGadgetList &FixableGadgets, + WarningGadgetList &WarningGadgets, + DeclUseTracker &Tracker) + : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets), + Tracker(Tracker) {} void run(const MatchFinder::MatchResult &Result) override { // In debug mode, assert that we've found exactly one gadget. @@ -2035,10 +2045,14 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler, assert(numFound >= 1 && "Gadgets not found in match result!"); assert(numFound <= 1 && "Conflicting bind tags in gadgets!"); } + + FixableGadgetList &FixableGadgets; + WarningGadgetList &WarningGadgets; + DeclUseTracker &Tracker; }; MatchFinder M; - GadgetFinderCallback CB; + GadgetFinderCallback CB{FixableGadgets, WarningGadgets, Tracker}; // clang-format off M.addMatcher( @@ -2083,9 +2097,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler, // clang-format on } - M.match(*D->getBody(), D->getASTContext()); - return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), - std::move(CB.Tracker)}; + M.match(*S, Ctx); } // Compares AST nodes by source locations. @@ -3630,39 +3642,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 @@ -3706,8 +3688,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; @@ -3928,3 +3912,56 @@ 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; + + if (const auto *FD = dyn_cast<FunctionDecl>(D)) { + // 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 *MD = dyn_cast<CXXMethodDecl>(D)) { + if (MD->getParent()->isLambda() && MD->getParent()->isLocalClass()) + return; + } + + for (FunctionDecl *FReDecl : FD->redecls()) { + if (FReDecl->isExternC()) { + // Do not emit fixit suggestions for functions declared in an + // extern "C" block. + 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()); + } + } + } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) { + Stmts.push_back(D->getBody()); + } + + assert(!Stmts.empty()); + + FixableGadgetList FixableGadgets; + WarningGadgetList WarningGadgets; + DeclUseTracker Tracker; + for (Stmt *S : Stmts) { + findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets, + WarningGadgets, Tracker); + } + applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets), + std::move(Tracker), Handler, EmitSuggestions); +} 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 bfc34b55c1f667..724d444638b57e 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,37 @@ 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 2{{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}} + {} +}; + // https://github.com/llvm/llvm-project/issues/80482 void testClassMembers() { UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}} @@ -122,4 +153,95 @@ 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}} +} + +// Not an aggregate, so its constructor is not implicit code and will be +// visited/checked for warnings. +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}} +}; + +// An aggregate, so its constructor is implicit code. Since it's not called, it +// is never generated. +struct AggregateUnused { + UnsafeMembers f1; + // While this field would trigger the warning during initialization, since + // it's unused, there's no code generated that does the initialization, so + // no warning. + UnsafeMembers f2{3}; +}; + +struct AggregateExplicitlyInitializedSafe { + UnsafeMembers f1; + // The warning is not fired as the field is always explicltly initialized + // elsewhere. This initializer is never used. + UnsafeMembers f2{3}; +}; + +void testAggregateExplicitlyInitializedSafe() { + AggregateExplicitlyInitializedSafe A{ + .f2 = UnsafeMembers(), // A safe constructor. + }; } + +struct AggregateExplicitlyInitializedUnsafe { + UnsafeMembers f1; + // The warning is not fired as the field is always explicltly initialized + // elsewhere. This initializer is never used. + UnsafeMembers f2{3}; +}; + +void testAggregateExplicitlyInitializedUnsafe() { + AggregateExplicitlyInitializedUnsafe A{ + .f2 = UnsafeMembers(3), // expected-warning{{function introduces unsafe buffer manipulation}} + }; +} + +struct AggregateViaAggregateInit { + UnsafeMembers f1; + // FIXME: A construction of this class does initialize the field through + // this initializer, so it should warn. Ideally it should also point to + // where the site of the construction is in testAggregateViaAggregateInit(). + UnsafeMembers f2{3}; +}; + +void testAggregateViaAggregateInit() { + AggregateViaAggregateInit A{}; +}; + +struct AggregateViaValueInit { + UnsafeMembers f1; + // FIXME: A construction of this class does initialize the field through + // this initializer, so it should warn. Ideally it should also point to + // where the site of the construction is in testAggregateViaValueInit(). + UnsafeMembers f2{3}; +}; + +void testAggregateViaValueInit() { + auto A = AggregateViaValueInit(); +}; + +struct AggregateViaDefaultInit { + UnsafeMembers f1; + // FIXME: A construction of this class does initialize the field through + // this initializer, so it should warn. Ideally it should also point to + // where the site of the construction is in testAggregateViaValueInit(). + UnsafeMembers f2{3}; +}; + +void testAggregateViaDefaultInit() { + AggregateViaDefaultInit A; +}; diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp index 638c76c58c0250..30b6d4ba9fb904 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp @@ -174,3 +174,23 @@ namespace test_flag { } } //namespace test_flag + +struct HoldsStdSpanAndInitializedInCtor { + char* Ptr; + unsigned Size; + std::span<char> Span{Ptr, Size}; // no-warning (this code is unreachable) + + HoldsStdSpanAndInitializedInCtor(char* P, unsigned S) + : Span(P, S) // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}} + {} +}; + +struct HoldsStdSpanAndNotInitializedInCtor { + char* Ptr; + unsigned Size; + std::span<char> Span{Ptr, Size}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}} + + HoldsStdSpanAndNotInitializedInCtor(char* P, unsigned S) + : Ptr(P), Size(S) + {} +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
