[-Wunsafe-buffer-usage] Fix false negatives of missing 2-param span ctors in constructor initializers

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

@@ -122,7 +122,12 @@ class UnsafeBufferUsageHandler {
                                     const Expr *UnsafeArg = nullptr) = 0;
 
   /// Invoked when an unsafe operation with a std container is found.
-  virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+  /// \param Invocation the `Stmt` that either is a direct call to the unsafe
+  /// span constructor or involves a direct call to it
+  /// \param UnsafeSpanCall the `Stmt` that refers to the direct call to the
+  /// unsafe span constructor
+  virtual void handleUnsafeOperationInContainer(const Stmt *Invocation,
+                                                const Stmt *UnsafeSpanCall,
                                                 bool IsRelatedToDecl,
                                                 ASTContext &Ctx) = 0;
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -12521,6 +12521,8 @@ def note_safe_buffer_usage_suggestions_disabled : Note<
 def warn_unsafe_buffer_usage_in_container : Warning<
   "the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information">,
   InGroup<UnsafeBufferUsageInContainer>, DefaultIgnore;
+def note_unsafe_buffer_usage_in_container : Note<
+  "the 'std::span' constructor call here">;
 #ifndef NDEBUG
 // Not a user-facing diagnostic. Useful for debugging false negatives in
 // -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -455,6 +455,28 @@ AST_MATCHER_P(CallExpr, hasNumArgs, unsigned, Num) {
   return Node.getNumArgs() == Num;
 }
 
+// Matches a CXXConstructor call if it matches the `InnerMatcher` in a recursive
+// manner. I.e., either
+//  1) `Node` matches `InnerMatcher` directly, or
+//  2) a descendant of one of `Node`'s initializers matches
+//   `ctorOrForEachInitializerDescendant` with the `InnerMatcher`.
+AST_MATCHER_P(CXXConstructExpr, ctorOrForEachInitializerDescendant,
+              internal::Matcher<CXXConstructExpr>, InnerMatcher) {
+  if (InnerMatcher.matches(Node, Finder, Builder))
+    return true;
+
+  const CXXConstructorDecl *CD = Node.getConstructor();
+
+  // recursive on both base and member initializers:
+  for (const CXXCtorInitializer *Init : CD->inits()) {
+    if (forEachDescendantStmt(
+            cxxConstructExpr(ctorOrForEachInitializerDescendant(InnerMatcher)))
+            .matches(*Init->getInit(), Finder, Builder))
+      return true;
+  }
+  return false;
+}
+
 namespace libc_func_matchers {
 // Under `libc_func_matchers`, define a set of matchers that match unsafe
 // functions in libc and unsafe calls to them.
@@ -1218,38 +1240,50 @@ class PointerArithmeticGadget : public WarningGadget {
 };
 
 class SpanTwoParamConstructorGadget : public WarningGadget {
-  static constexpr const char *const SpanTwoParamConstructorTag =
-      "spanTwoParamConstructor";
-  const CXXConstructExpr *Ctor; // the span constructor expression
+  static constexpr const char *const InvocationExprTag =
+      "spanTwoParamConstructor_Invocation";
+  static constexpr const char *const SpanTwoParamCtorTag =
+      "spanTwoParamConstructor_Ctor";
+  // The constructor call that either 1) is a direct call to the unsafe span
+  // constructor, or 2) involves a call to the unsafe span constructor
+  // (recursively through constructor initializers):
+  const CXXConstructExpr *Invocation;
+  // In case 1) above, `Ctor == Invocation`; otherwise `Ctor` refers to the
+  // actual span constructor call involved in the `Invocation`.
+  const CXXConstructExpr *Ctor;
 
 public:
   SpanTwoParamConstructorGadget(const MatchFinder::MatchResult &Result)
       : WarningGadget(Kind::SpanTwoParamConstructor),
-        Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(
-            SpanTwoParamConstructorTag)) {}
+        Invocation(Result.Nodes.getNodeAs<CXXConstructExpr>(InvocationExprTag)),
+        Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(SpanTwoParamCtorTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::SpanTwoParamConstructor;
   }
 
   static Matcher matcher() {
-    auto HasTwoParamSpanCtorDecl = hasDeclaration(
-        cxxConstructorDecl(hasDeclContext(isInStdNamespace()), hasName("span"),
-                           parameterCountIs(2)));
-
-    return stmt(cxxConstructExpr(HasTwoParamSpanCtorDecl,
-                                 unless(isSafeSpanTwoParamConstruct()))
-                    .bind(SpanTwoParamConstructorTag));
+    auto HasTwoParamSpanCtorDecl =
+        cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+                             hasDeclContext(isInStdNamespace()),
+                             hasName("span"), parameterCountIs(2))),
+                         unless(isSafeSpanTwoParamConstruct()))
+            .bind(SpanTwoParamCtorTag);
+    return stmt(cxxConstructExpr(
+                    ctorOrForEachInitializerDescendant(HasTwoParamSpanCtorDecl))
+                    .bind(InvocationExprTag));
   }
 
   static Matcher matcher(const UnsafeBufferUsageHandler *Handler) {
-    return stmt(unless(ignoreUnsafeBufferInContainer(Handler)), matcher());
+    return stmt(unless(ignoreUnsafeBufferInContainer(Handler)),
+                SpanTwoParamConstructorGadget::matcher());
   }
 
   void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
                              bool IsRelatedToDecl,
                              ASTContext &Ctx) const override {
-    Handler.handleUnsafeOperationInContainer(Ctor, IsRelatedToDecl, Ctx);
+    Handler.handleUnsafeOperationInContainer(Invocation, Ctor, IsRelatedToDecl,
+                                             Ctx);
   }
   SourceLocation getSourceLoc() const override { return Ctor->getBeginLoc(); }
 

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2327,19 +2327,21 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
     }
   }
 
-  void handleUnsafeOperationInContainer(const Stmt *Operation,
+  void handleUnsafeOperationInContainer(const Stmt *Invocation,
+                                        const Stmt *UnsafeSpanCall,
                                         bool IsRelatedToDecl,
                                         ASTContext &Ctx) override {
     SourceLocation Loc;
     SourceRange Range;
     unsigned MsgParam = 0;
 
-    // This function only handles SpanTwoParamConstructorGadget so far, which
-    // always gives a CXXConstructExpr.
-    const auto *CtorExpr = cast<CXXConstructExpr>(Operation);
-    Loc = CtorExpr->getLocation();
-
-    S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container);
+    Loc = Invocation->getBeginLoc();
+    S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container)
+        << Invocation->getSourceRange();
+    if (Invocation != UnsafeSpanCall)
+      S.Diag(UnsafeSpanCall->getBeginLoc(),
+             diag::note_unsafe_buffer_usage_in_container)
+          << UnsafeSpanCall->getSourceRange();
     if (IsRelatedToDecl) {
       assert(!SuggestSuggestions &&
              "Variables blamed for unsafe buffer usage without suggestions!");

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

@@ -157,3 +157,61 @@ namespace test_flag {
 
   }
 } //namespace test_flag
+
+
+namespace ctor_initializer_list {
+  class Base {
+    std::span<int> S;
+
+  public:
+    Base(int *p, unsigned n) : S(p, n) {} // expected-note 4{{the 'std::span' constructor call here}}
+  };
+
+  class Derived : public Base {
+  public:
+    Derived(int *p, unsigned n) : Base(p, n) {}
+  };
+
+  class Friend {
+    Derived D;
+  public:
+    Friend(int *p, unsigned n) : D(p, n) {}
+  };
+
+  class FriendToo {
+    Friend F;
+  public:
+    FriendToo(int *p) : F(p, 10) {}
+  };
+
+  class SpanData {
+    int * P;
+  public:
+    SpanData(int *p) : P(std::span<int>(p, 10).data()) {} // expected-note 2{{the 'std::span' constructor call here}}
+  };
+
+  class SpanDataDerived : public SpanData {
+  public:
+    SpanDataDerived(int *p) : SpanData(p) {}
+  };
+
+  void foo(int *p) {
+    Base B(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+    Derived D(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+    Friend F(p, 10); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+    FriendToo FT(p); // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+    SpanData SD(p);  // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+    SpanDataDerived SDD(p);  // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunsafe-buffer-usage-in-container"
+    {
+      Base B(p, 10);
+      Derived D(p, 10);
+      Friend F(p, 10);
+      FriendToo FT(p);
+      SpanData SD(p);
+      SpanDataDerived SDD(p);
+    }
+#pragma clang diagnostic pop
+  }
+}  // ctor_initializer_list