[-Wunsafe-buffer-usage] Warn string_view constructions that not guarantee null-termination

One could use `string_view` as the safe view over strings in read-only
scenarios as it is hardened, preserving bounds info and guaranteeing
null-termination with the new warning.

If one finds the warning being too restrictive, turn it off with
`-Wno-unsafe-buffer-usage-in-string-view`.  The rest of the warnings
will not assume null-termination guarantee of `string_view` as well
then.

Author: ziqingluo-90

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

@@ -147,10 +147,16 @@ class UnsafeBufferUsageHandler {
   virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0;
 
   /// \return true iff unsafe uses in containers should NOT be reported at
-  /// `Loc`; false otherwise.
+  /// `Loc`; false otherwise. Controlled by `-Wunsafe-buffer-usage-in-container`
   virtual bool
   ignoreUnsafeBufferInContainer(const SourceLocation &Loc) const = 0;
 
+  /// \return true iff unsafe uses in string_views should NOT be reported at
+  /// `Loc`; false otherwise. Controlled by
+  /// `-Wunsafe-buffer-usage-in-string-view`
+  virtual bool
+  ignoreUnsafeBufferInStringView(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
 
+/// Make `WARNING_LIBC_GADGET` self a subset of WARNING_GADGET so that it can be
+/// tuned specially according to whether we warn `StringViewUnsafeConstructor`.
+#ifndef WARNING_LIBC_GADGET
+#define WARNING_LIBC_GADGET(name) WARNING_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
@@ -38,8 +44,9 @@ WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 WARNING_GADGET(UnsafeBufferUsageCtorAttr)
 WARNING_GADGET(DataInvocation)
-WARNING_GADGET(UnsafeLibcFunctionCall)
+WARNING_LIBC_GADGET(UnsafeLibcFunctionCall)
 WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)`
+WARNING_CONTAINER_GADGET(StringViewUnsafeConstructor) // `std::string_view` constructed from std::string guarantees null-termination
 FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalue Context
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
 FIXABLE_GADGET(PointerDereference)
@@ -53,5 +60,7 @@ FIXABLE_GADGET(PointerInit)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
+#undef WARNING_LIBC_GADGET
 #undef WARNING_CONTAINER_GADGET
+#undef WARNING_STRINGVIEW_GADGET
 #undef GADGET

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1551,7 +1551,8 @@ def HLSLAvailability : DiagGroup<"hlsl-availability">;
 def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">;
 
 // Warnings and fixes to support the "safe buffers" programming model.
-def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container">;
+def UnsafeBufferUsageInStringView : DiagGroup<"unsafe-buffer-usage-in-string-view">;
+def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container", [UnsafeBufferUsageInStringView]>;
 def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer]>;
 
 // Warnings and notes related to the function effects system underlying

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -12388,6 +12388,9 @@ 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 warn_unsafe_buffer_usage_in_string_view : Warning<
+  "construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead">,
+  InGroup<UnsafeBufferUsageInStringView>, 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

@@ -247,11 +247,16 @@ AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
   return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
 }
 
-AST_MATCHER_P(Stmt, ignoreUnsafeBufferInContainer,
+AST_MATCHER_P(Stmt, ignoreSpanTwoParamConstructor,
               const UnsafeBufferUsageHandler *, Handler) {
   return Handler->ignoreUnsafeBufferInContainer(Node.getBeginLoc());
 }
 
+AST_MATCHER_P(Stmt, ignoreStringViewUnsafeConstructor,
+              const UnsafeBufferUsageHandler *, Handler) {
+  return Handler->ignoreUnsafeBufferInStringView(Node.getBeginLoc());
+}
+
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
   return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
 }
@@ -448,7 +453,8 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
   return false;
 }
 
-AST_MATCHER(CallExpr, isUnsafeLibcFunctionCall) {
+AST_MATCHER_P(CallExpr, isUnsafeLibcFunctionCall, internal::Matcher<Expr>,
+              hasUnsafeStringView) {
   static const std::set<StringRef> PredefinedNames{
       // numeric conversion:
       "atof",
@@ -525,7 +531,9 @@ AST_MATCHER(CallExpr, isUnsafeLibcFunctionCall) {
   // checks for `printf`s:
   struct FuncNameMatch {
     const CallExpr *const Call;
-    FuncNameMatch(const CallExpr *Call) : Call(Call) {}
+    const bool hasUnsafeStringView;
+    FuncNameMatch(const CallExpr *Call, bool hasUnsafeStringView)
+        : Call(Call), hasUnsafeStringView(hasUnsafeStringView) {}
 
     // For a name `S` in `PredefinedNames` or a member of the printf/scanf
     // family, define matching function names with `S` by the grammar below:
@@ -605,27 +613,35 @@ AST_MATCHER(CallExpr, isUnsafeLibcFunctionCall) {
 
     // A pointer type expression is known to be null-terminated, if it has the
     // form: E.c_str(), for any expression E of `std::string` type.
-    static bool isNullTermPointer(const Expr *Ptr) {
+    static bool isNullTermPointer(const Expr *Ptr,
+                                  bool UnsafeStringView = true) {
       if (isa<StringLiteral>(Ptr->IgnoreParenImpCasts()))
         return true;
       if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Ptr->IgnoreParenImpCasts())) {
         const CXXMethodDecl *MD = MCE->getMethodDecl();
         const CXXRecordDecl *RD = MCE->getRecordDecl()->getCanonicalDecl();
 
-        if (MD && RD && RD->isInStdNamespace())
+        if (MD && RD && RD->isInStdNamespace()) {
           if (MD->getName() == "c_str" && RD->getName() == "basic_string")
             return true;
+          if (!UnsafeStringView && MD->getName() == "data" &&
+              RD->getName() == "basic_string_view")
+            return true;
+        }
       }
       return false;
     }
 
     // Check safe patterns for printfs w.r.t their prefixes:
     bool isUnsafePrintf(StringRef Prefix /* empty, 'k', 'f', 's', or 'sn' */) {
-      auto AnyUnsafeStrPtr = [](const Expr *Arg) -> bool {
-        return Arg->getType()->isPointerType() && !isNullTermPointer(Arg);
+      const bool hasUnsafeStringView = this->hasUnsafeStringView;
+      auto AnyUnsafeStrPtr = [&hasUnsafeStringView](const Expr *Arg) -> bool {
+        return Arg->getType()->isPointerType() &&
+               !isNullTermPointer(Arg, hasUnsafeStringView);
       };
 
-      if (Prefix.empty() || Prefix == "k") // printf: all pointer args should be null-terminated
+      if (Prefix.empty() ||
+          Prefix == "k") // printf: all pointer args should be null-terminated
         return llvm::any_of(Call->arguments(), AnyUnsafeStrPtr);
 
       if (Prefix == "f" && Call->getNumArgs() > 1) {
@@ -677,7 +693,7 @@ AST_MATCHER(CallExpr, isUnsafeLibcFunctionCall) {
       return Name == "strlen" && Call->getNumArgs() == 1 &&
              isa<StringLiteral>(Call->getArg(0)->IgnoreParenImpCasts());
     }
-  } FuncNameMatch{&Node};
+  } FuncNameMatch{&Node, hasUnsafeStringView.matches(Node, Finder, Builder)};
 
   const FunctionDecl *FD = Node.getDirectCallee();
   const IdentifierInfo *II;
@@ -1037,6 +1053,47 @@ class SpanTwoParamConstructorGadget : public WarningGadget {
   }
 };
 
+class StringViewUnsafeConstructorGadget : public WarningGadget {
+  static constexpr const char *const Tag = "StringViewUnsafeConstructor";
+  const CXXConstructExpr *Ctor;
+
+public:
+  StringViewUnsafeConstructorGadget(const MatchFinder::MatchResult &Result)
+      : WarningGadget(Kind::StringViewUnsafeConstructor),
+        Ctor(Result.Nodes.getNodeAs<CXXConstructExpr>(Tag)) {}
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::StringViewUnsafeConstructor;
+  }
+
+  // Matches any `basic_string_view` constructor call that is not a copy
+  // constructor or on a string literal:
+  static Matcher matcher() {
+    auto isStringViewDecl = hasDeclaration(cxxConstructorDecl(
+        hasDeclContext(isInStdNamespace()), hasName("basic_string_view")));
+
+    return stmt(
+        cxxConstructExpr(
+            isStringViewDecl,
+            unless(
+                hasArgument(0, expr(ignoringParenImpCasts(stringLiteral())))),
+            unless(hasDeclaration(cxxConstructorDecl(isCopyConstructor()))))
+            .bind(Tag));
+  }
+
+  const Stmt *getBaseStmt() const override { return Ctor; }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    // If the constructor call is of the form `std::basic_string_view{ptrVar,
+    // ...}`, `ptrVar` is considered an unsafe variable.
+    if (auto *DRE = dyn_cast<DeclRefExpr>(Ctor->getArg(0))) {
+      if (DRE->getType()->isPointerType() && isa<VarDecl>(DRE->getDecl()))
+        return {DRE};
+    }
+    return {};
+  }
+};
+
 /// A pointer initialization expression of the form:
 ///  \code
 ///  int *p = q;
@@ -1293,8 +1350,10 @@ class UnsafeLibcFunctionCallGadget : public WarningGadget {
       : WarningGadget(Kind::UnsafeLibcFunctionCall),
         Call(Result.Nodes.getNodeAs<CallExpr>(Tag)) {}
 
-  static Matcher matcher() {
-    return stmt(callExpr(isUnsafeLibcFunctionCall()).bind(Tag));
+  static Matcher
+  matcher(ast_matchers::internal::Matcher<Expr> HasUnsafeStringView) {
+    return stmt(
+        callExpr(isUnsafeLibcFunctionCall(HasUnsafeStringView)).bind(Tag));
   }
 
   const Stmt *getBaseStmt() const override { return Call; }
@@ -1724,10 +1783,13 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
 #define WARNING_GADGET(x)                                                      \
           allOf(x ## Gadget::matcher().bind(#x),                               \
                 notInSafeBufferOptOut(&Handler)),
+#define WARNING_LIBC_GADGET(x)                                                          \
+          allOf(x ## Gadget::matcher(ignoreStringViewUnsafeConstructor(&Handler)).bind(#x), \
+                notInSafeBufferOptOut(&Handler)),
 #define WARNING_CONTAINER_GADGET(x)                                            \
           allOf(x ## Gadget::matcher().bind(#x),                               \
                 notInSafeBufferOptOut(&Handler),                               \
-                unless(ignoreUnsafeBufferInContainer(&Handler))),
+                unless(ignore ## x(&Handler))),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
             // Avoid a hanging comma.
             unless(stmt())

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2256,6 +2256,17 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
         Range = UO->getSubExpr()->getSourceRange();
         MsgParam = 1;
       }
+    } else if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) {
+      if (CtorExpr->getConstructor()->getCanonicalDecl()->getNameAsString() ==
+          "span")
+        S.Diag(CtorExpr->getLocation(),
+               diag::warn_unsafe_buffer_usage_in_container)
+            << CtorExpr->getSourceRange();
+      else // it is warning about "string_view":
+        S.Diag(CtorExpr->getLocation(),
+               diag::warn_unsafe_buffer_usage_in_string_view)
+            << CtorExpr->getSourceRange();
+      return;
     } else {
       if (isa<CallExpr>(Operation) || isa<CXXConstructExpr>(Operation)) {
         // note_unsafe_buffer_operation doesn't have this mode yet.
@@ -2370,6 +2381,12 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
     return S.Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container, Loc);
   }
 
+  bool
+  ignoreUnsafeBufferInStringView(const SourceLocation &Loc) const override {
+    return S.Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_string_view,
+                             Loc);
+  }
+
   // Returns the text representation of clang::unsafe_buffer_usage attribute.
   // `WSSuffix` holds customized "white-space"s, e.g., newline or whilespace
   // characters.

clang/test/SemaCXX/warn-unsafe-buffer-usage-in-string_view.cpp

@@ -0,0 +1,85 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s -Wno-unsafe-buffer-usage-in-string-view -D_IGNORE_STRING_VIEW
+
+namespace std {
+  struct iterator{};
+
+  template<typename T>
+  struct basic_string_view {
+    T* p;
+    T *data();
+    unsigned size_bytes();
+    iterator begin();
+    iterator end();
+
+    constexpr basic_string_view( const T* s ){};
+    constexpr basic_string_view( const T* s, unsigned count );
+    constexpr basic_string_view( const basic_string_view& other ) noexcept = default;
+    template< class It, class End >
+    constexpr basic_string_view( It first, End last ) {};
+
+  };
+
+  typedef basic_string_view<char> string_view;
+  typedef basic_string_view<wchar_t> wstring_view;
+
+  template<typename T>
+  struct basic_string {
+    T *c_str();
+    operator std::basic_string_view<T>() {
+    }
+  };
+
+  typedef basic_string<char> string;
+  typedef basic_string<wchar_t> wstring;
+
+  template <class T> struct span {
+    constexpr span(T *, unsigned){}
+  };
+}
+
+void foo(std::string_view);
+void f(char * p, std::string S, std::string_view V) {
+  std::string_view SVs[] {S, "S"};
+  foo(S);
+  foo("S");
+  foo({S});
+  foo({"S"});
+  foo(V);
+#ifndef _IGNORE_STRING_VIEW
+  foo(p);       // expected-warning{{construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead}}
+#else
+  foo(p);       // no warn
+#endif
+#ifndef _IGNORE_STRING_VIEW
+  foo({p, 10}); // expected-warning{{construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead}}
+#else
+  foo({p, 10}); // no warn
+#endif
+
+  std::string_view SV{S};
+  std::string_view SV2{"S"};
+  std::string_view SV3 = S;
+  std::string_view SV4 = "S";
+  std::string_view SV5 = std::string_view(S);
+#ifndef _IGNORE_STRING_VIEW
+  std::string_view SV10 = p; // expected-warning{{construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead}}
+#else
+  std::string_view SV10 = p; // no warn
+#endif
+#ifndef _IGNORE_STRING_VIEW
+  std::string_view SV11{p};  // expected-warning{{construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead}}
+#else
+  std::string_view SV11{p};  // no warn
+#endif
+#ifndef _IGNORE_STRING_VIEW
+  std::string_view SV12{V.begin(), V.end()}; // expected-warning{{construct string_view from raw pointers does not guarantee null-termination, construct from std::string instead}}
+#else
+  std::string_view SV12{V.begin(), V.end()}; // no warn
+#endif
+}
+
+void g(int * p) {
+  // This warning is not affected:
+  std::span<int> S{p, 10}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+}

clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp

@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
-// RUN:            -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s -Wno-unsafe-buffer-usage-in-string-view -D_IGNORE_STRING_VIEW
 
 typedef struct {} FILE;
 void memcpy();
@@ -42,6 +42,15 @@ namespace std {
 
   typedef basic_string<char> string;
   typedef basic_string<wchar_t> wstring;
+
+  template<typename T>
+  struct basic_string_view {
+    T* p;
+    T *data();
+    unsigned size_bytes();
+  };
+
+  typedef basic_string_view<char> string_view;
 }
 
 void f(char * p, char * q, std::span<char> s) {
@@ -69,8 +78,13 @@ void f(char * p, char * q, std::span<char> s) {
   strlen("hello");// no warn
 }
 
-void v(std::string s1, std::wstring s2) {
-  snprintf(s1.data(), s1.size_bytes(), "%s%d", s1.c_str(), 0); // no warn
+void v(std::string s, std::string_view sv) {
+  snprintf(s.data(), s.size_bytes(), "%s%d", s.c_str(), 0); // no warn
+#ifndef _IGNORE_STRING_VIEW
+  snprintf(sv.data(), sv.size_bytes(), "%s%d", sv.data(), 0); // no warn
+#else
+  snprintf(sv.data(), sv.size_bytes(), "%s%d", sv.data(), 0); // expected-warning{{function introduces unsafe buffer manipulation}} expected-note{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+#endif
 }