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}} };

Author: danakj

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);
+ }
+}

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

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}}
+};

clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp

@@ -154,3 +154,13 @@ namespace test_flag {
 
  }
 } //namespace test_flag
+
+struct HoldsStdSpan {
+ 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}}
+
+ HoldsStdSpan(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}}
+ {}
+};