[clang] c60b055 - Reapply "Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (#91991)"
Zequan Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 27 11:18:22 PST 2024
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)
+ {}
+};
More information about the cfe-commits
mailing list