[-Wunsafe-buffer-usage] Respect the attribute for field and constructor initializers (llvm#91991) (#9537)

CXXCtorInitializers are not statements , 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) {}
}

Field initializers can be found by traversing CXXDefaultInitExprs. This
catches warnings in places such as:

struct C {
  P* Ptr;
  AnUnsafeCtor U{Ptr};
};

We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.

Note that aggregate initialization is not fully covered where a field
specifies an initializer and it's not overridden in the aggregate initialization,
such as in:

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

There are 3 tests for different types of aggregate initialization with
FIXMEs documenting this future work.

One attempt to fix this involved returning true from
MatchDescendantVisitor::shouldVisitImplicitCode(), however, it 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}}
};

Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.

Issue llvm#80482

(cherry picked from commit a518ed2)

Co-authored-by: Dana Jansens <[email protected]>

(rdar://137999201)

Author: ziqingluo-90

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -171,6 +171,12 @@ class MatchDescendantVisitor
     return VisitorBase::TraverseCXXTypeidExpr(Node);
   }
 
+  bool TraverseCXXDefaultInitExpr(CXXDefaultInitExpr *Node) {
+    if (!TraverseStmt(Node->getExpr()))
+      return false;
+    return VisitorBase::TraverseCXXDefaultInitExpr(Node);
+  }
+
   bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
     if (!Node)
       return true;
@@ -1969,14 +1975,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.
@@ -2017,10 +2027,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(
@@ -2065,9 +2079,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.
@@ -3612,39 +3624,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
@@ -3688,8 +3670,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;
 
@@ -3910,3 +3894,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);
+}

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

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

@@ -154,3 +154,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)
+  {}
+};