[-Wunsafe-buffer-usage] Add a new warning for uses of std::span two-p…

…arameter constructors (#77148)

Constructing `std::span` objects with the two parameter constructors
could introduce mismatched bounds information, which defeats the
purpose of using `std::span`.  Therefore, we warn every use of such
constructors.

rdar://115817781

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

@@ -16,6 +16,7 @@
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/Support/Debug.h"
 
 namespace clang {
@@ -98,9 +99,14 @@ class UnsafeBufferUsageHandler {
 #endif
 
 public:
-  /// Returns a reference to the `Preprocessor`:
+  /// \return true iff buffer safety is opt-out at `Loc`; false otherwise.
   virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
 
+  /// \return true iff unsafe uses in containers should NOT be reported at
+  /// `Loc`; false otherwise.
+  virtual bool
+  ignoreUnsafeBufferInContainer(const SourceLocation &Loc) const = 0;
+
   virtual std::string
   getUnsafeBufferUsageAttributeTextAt(SourceLocation Loc,
                                       StringRef WSSuffix = "") const = 0;

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

@@ -18,6 +18,12 @@
 #define WARNING_GADGET(name) GADGET(name)
 #endif
 
+/// A `WARNING_GADGET` subset, where the code pattern of each gadget
+/// corresponds uses of a (possibly hardened) contatiner (e.g., `std::span`).
+#ifndef WARNING_CONTAINER_GADGET
+#define WARNING_CONTAINER_GADGET(name) WARNING_GADGET(name)
+#endif
+
 /// Safe gadgets correspond to code patterns that aren't unsafe but need to be
 /// properly recognized in order to emit correct warnings and fixes over unsafe
 /// gadgets.
@@ -31,6 +37,7 @@ WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 WARNING_GADGET(DataInvocation)
+WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)`
 FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalue Context
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
 FIXABLE_GADGET(PointerDereference)
@@ -43,4 +50,5 @@ FIXABLE_GADGET(PointerInit)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
+#undef WARNING_CONTAINER_GADGET
 #undef GADGET

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -12111,6 +12111,9 @@ def note_unsafe_buffer_variable_fixit_together : Note<
   "%select{|, and change %2 to safe types to make function %4 bounds-safe}3">;
 def note_safe_buffer_usage_suggestions_disabled : Note<
   "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
+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;
 #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

@@ -14,11 +14,12 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
 #include <memory>
 #include <optional>
-#include <sstream>
 #include <queue>
+#include <sstream>
 
 using namespace llvm;
 using namespace clang;
@@ -232,6 +233,11 @@ AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
   return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
 }
 
+AST_MATCHER_P(Stmt, ignoreUnsafeBufferInContainer,
+              const UnsafeBufferUsageHandler *, Handler) {
+  return Handler->ignoreUnsafeBufferInContainer(Node.getBeginLoc());
+}
+
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
   return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
 }
@@ -325,6 +331,73 @@ isInUnspecifiedUntypedContext(internal::Matcher<Stmt> InnerMatcher) {
   // FIXME: Handle loop bodies.
   return stmt(anyOf(CompStmt, IfStmtThen, IfStmtElse));
 }
+
+// Given a two-param std::span construct call, matches iff the call has the
+// following forms:
+//   1. `std::span<T>{new T[n], n}`, where `n` is a literal or a DRE
+//   2. `std::span<T>{new T, 1}`
+//   3. `std::span<T>{&var, 1}`
+//   4. `std::span<T>{a, n}`, where `a` is of an array-of-T with constant size
+//   `n`
+//   5. `std::span<T>{any, 0}`
+AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
+  assert(Node.getNumArgs() == 2 &&
+         "expecting a two-parameter std::span constructor");
+  const Expr *Arg0 = Node.getArg(0)->IgnoreImplicit();
+  const Expr *Arg1 = Node.getArg(1)->IgnoreImplicit();
+  auto HaveEqualConstantValues = [&Finder](const Expr *E0, const Expr *E1) {
+    if (auto E0CV = E0->getIntegerConstantExpr(Finder->getASTContext()))
+      if (auto E1CV = E1->getIntegerConstantExpr(Finder->getASTContext())) {
+        return APSInt::compareValues(*E0CV, *E1CV) == 0;
+      }
+    return false;
+  };
+  auto AreSameDRE = [](const Expr *E0, const Expr *E1) {
+    if (auto *DRE0 = dyn_cast<DeclRefExpr>(E0))
+      if (auto *DRE1 = dyn_cast<DeclRefExpr>(E1)) {
+        return DRE0->getDecl() == DRE1->getDecl();
+      }
+    return false;
+  };
+  std::optional<APSInt> Arg1CV =
+      Arg1->getIntegerConstantExpr(Finder->getASTContext());
+
+  if (Arg1CV && Arg1CV->isZero())
+    // Check form 5:
+    return true;
+  switch (Arg0->IgnoreImplicit()->getStmtClass()) {
+  case Stmt::CXXNewExprClass:
+    if (auto Size = cast<CXXNewExpr>(Arg0)->getArraySize()) {
+      // Check form 1:
+      return AreSameDRE((*Size)->IgnoreImplicit(), Arg1) ||
+             HaveEqualConstantValues(*Size, Arg1);
+    }
+    // TODO: what's placeholder type? avoid it for now.
+    if (!cast<CXXNewExpr>(Arg0)->hasPlaceholderType()) {
+      // Check form 2:
+      return Arg1CV && Arg1CV->isOne();
+    }
+    break;
+  case Stmt::UnaryOperatorClass:
+    if (cast<UnaryOperator>(Arg0)->getOpcode() ==
+        UnaryOperator::Opcode::UO_AddrOf)
+      // Check form 3:
+      return Arg1CV && Arg1CV->isOne();
+    break;
+  default:
+    break;
+  }
+
+  QualType Arg0Ty = Arg0->IgnoreImplicit()->getType();
+
+  if (Arg0Ty->isConstantArrayType()) {
+    const APInt &ConstArrSize = cast<ConstantArrayType>(Arg0Ty)->getSize();
+
+    // Check form 4:
+    return Arg1CV && APSInt::compareValues(APSInt(ConstArrSize), *Arg1CV) == 0;
+  }
+  return false;
+}
 } // namespace clang::ast_matchers
 
 namespace {
@@ -594,6 +667,44 @@ class PointerArithmeticGadget : public WarningGadget {
   // FIXME: this gadge will need a fix-it
 };
 
+class SpanTwoParamConstructorGadget : public WarningGadget {
+  static constexpr const char *const SpanTwoParamConstructorTag =
+      "spanTwoParamConstructor";
+  const CXXConstructExpr *Ctor; // the span constructor expression
+
+public:
+  SpanTwoParamConstructorGadget(const MatchFinder::MatchResult &Result)
+      : WarningGadget(Kind::SpanTwoParamConstructor),
+        Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(
+            SpanTwoParamConstructorTag)) {}
+
+  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));
+  }
+
+  const Stmt *getBaseStmt() const override { return Ctor; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    // If the constructor call is of the form `std::span{var, n}`, `var` is
+    // considered an unsafe variable.
+    if (auto *DRE = dyn_cast<DeclRefExpr>(Ctor->getArg(0))) {
+      if (isa<VarDecl>(DRE->getDecl()))
+        return {DRE};
+    }
+    return {};
+  }
+};
+
 /// A pointer initialization expression of the form:
 ///  \code
 ///  int *p = q;
@@ -1210,6 +1321,10 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
 #define WARNING_GADGET(x)                                                      \
           allOf(x ## Gadget::matcher().bind(#x),                               \
                 notInSafeBufferOptOut(&Handler)),
+#define WARNING_CONTAINER_GADGET(x)                                            \
+          allOf(x ## Gadget::matcher().bind(#x),                               \
+                notInSafeBufferOptOut(&Handler),                               \
+                unless(ignoreUnsafeBufferInContainer(&Handler))),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
             // Avoid a hanging comma.
             unless(stmt())

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -26,7 +26,6 @@
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/StmtVisitor.h"
-#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
 #include "clang/Analysis/Analyses/CalledOnceCheck.h"
@@ -39,6 +38,7 @@
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CFGStmtMap.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Preprocessor.h"
@@ -2256,6 +2256,9 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
         Range = UO->getSubExpr()->getSourceRange();
         MsgParam = 1;
       }
+    } else if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) {
+      S.Diag(CtorExpr->getLocation(),
+             diag::warn_unsafe_buffer_usage_in_container);
     } else {
       if (isa<CallExpr>(Operation)) {
         // note_unsafe_buffer_operation doesn't have this mode yet.
@@ -2334,6 +2337,10 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
     return S.PP.isSafeBufferOptOut(S.getSourceManager(), Loc);
   }
 
+  bool ignoreUnsafeBufferInContainer(const SourceLocation &Loc) const override {
+    return S.Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container, Loc);
+  }
+
   // Returns the text representation of clang::unsafe_buffer_usage attribute.
   // `WSSuffix` holds customized "white-space"s, e.g., newline or whilespace
   // characters.
@@ -2498,6 +2505,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
     if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation,
                          Node->getBeginLoc()) ||
         !Diags.isIgnored(diag::warn_unsafe_buffer_variable,
+                         Node->getBeginLoc()) ||
+        !Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
                          Node->getBeginLoc())) {
       clang::checkUnsafeBufferUsage(Node, R,
                                     UnsafeBufferUsageShouldEmitSuggestions);
@@ -2508,7 +2517,9 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
   // Emit per-function analysis-based warnings that require the whole-TU
   // reasoning. Check if any of them is enabled at all before scanning the AST:
   if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) ||
-      !Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) {
+      !Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation()) ||
+      !Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
+                       SourceLocation())) {
     CallableVisitor(CallAnalyzers).TraverseTranslationUnitDecl(TU);
   }
 }