[clang] Warn when size arguments miss the null terminator (PR #190447)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Pppp1116 (Pppp1116)

<details>
<summary>Changes</summary>



---
Full diff: https://github.com/llvm/llvm-project/pull/190447.diff


3 Files Affected:

- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4) 
- (modified) clang/lib/Sema/SemaChecking.cpp (+101) 
- (added) clang/test/Sema/warn-fortify-missing-null-terminator-space.c (+19) 


``````````diff
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index eddf9c50033e1..adab998cd7d7f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -965,6 +965,10 @@ def warn_fortify_strlen_overflow: Warning<
   " but the source string has length %2 (including NUL byte)">,
   InGroup<FortifySource>;
 
+def warn_fortify_missing_null_terminator_space : Warning<
+  "%select{allocation size|copy size}0 does not include space for null "
+  "terminator; consider '%1 + 1'">, InGroup<FortifySource>;
+
 def subst_format_overflow : TextSubstitution<
   "'%0' will always overflow; destination buffer has size %1,"
   " but format string expands to at least %2">;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index de8b965144971..9a207a407e28c 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -1141,6 +1141,106 @@ static bool ProcessFormatStringLiteral(const Expr *FormatExpr,
   return false;
 }
 
+static unsigned getDiagnosedBuiltinID(const FunctionDecl *FD) {
+  if (!FD)
+    return 0;
+
+  if (const auto *DABAttr = FD->getAttr<DiagnoseAsBuiltinAttr>())
+    FD = DABAttr->getFunction();
+
+  return FD ? FD->getBuiltinID(/*ConsiderWrappers=*/true) : 0;
+}
+
+static const Expr *getDirectStrlenCallArgument(const Expr *E) {
+  const auto *CE = dyn_cast<CallExpr>(E->IgnoreParenCasts());
+  if (!CE || CE->getNumArgs() != 1)
+    return nullptr;
+
+  switch (getDiagnosedBuiltinID(CE->getDirectCallee())) {
+  case Builtin::BIstrlen:
+  case Builtin::BI__builtin_strlen:
+    return CE->getArg(0)->IgnoreParenCasts();
+  default:
+    return nullptr;
+  }
+}
+
+static bool areSameSimpleDeclRef(const Expr *E1, const Expr *E2) {
+  const auto *D1 = dyn_cast<DeclRefExpr>(E1->IgnoreParenCasts());
+  const auto *D2 = dyn_cast<DeclRefExpr>(E2->IgnoreParenCasts());
+  return D1 && D2 && D1->getDecl() == D2->getDecl();
+}
+
+static std::string getExprAsString(const Expr *E,
+                                   const PrintingPolicy &Policy) {
+  SmallString<64> Text;
+  llvm::raw_svector_ostream OS(Text);
+  E->printPretty(OS, nullptr, Policy);
+  return std::string(OS.str());
+}
+
+static FixItHint getPlusOneFixIt(const Expr *E, const PrintingPolicy &Policy) {
+  const Expr *SimpleExpr = E->IgnoreParenCasts();
+  if (!isa<DeclRefExpr, CallExpr>(SimpleExpr))
+    return {};
+
+  SourceRange Range = E->getSourceRange();
+  if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
+    return {};
+
+  SmallString<64> Replacement;
+  llvm::raw_svector_ostream OS(Replacement);
+  E->printPretty(OS, nullptr, Policy);
+  OS << " + 1";
+  return FixItHint::CreateReplacement(Range, OS.str());
+}
+
+static void DiagnoseMissingNullTerminatorSpace(Sema &S, SourceLocation Loc,
+                                               const Expr *SizeExpr,
+                                               unsigned MissingKind) {
+  std::string SizeText = getExprAsString(SizeExpr, S.getPrintingPolicy());
+  S.Diag(Loc, diag::warn_fortify_missing_null_terminator_space)
+      << MissingKind << SizeText
+      << getPlusOneFixIt(SizeExpr, S.getPrintingPolicy());
+}
+
+static void CheckCStringNullTerminatorSizeArguments(Sema &S,
+                                                    const FunctionDecl *FD,
+                                                    const CallExpr *TheCall) {
+  if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
+      S.isConstantEvaluatedContext() ||
+      S.Diags.isIgnored(diag::warn_fortify_missing_null_terminator_space,
+                        TheCall->getBeginLoc()))
+    return;
+
+  switch (getDiagnosedBuiltinID(FD)) {
+  case Builtin::BImalloc:
+  case Builtin::BI__builtin_malloc: {
+    if (TheCall->getNumArgs() != 1)
+      return;
+    const Expr *SizeArg = TheCall->getArg(0)->IgnoreParenCasts();
+    if (getDirectStrlenCallArgument(SizeArg))
+      DiagnoseMissingNullTerminatorSpace(S, TheCall->getBeginLoc(), SizeArg,
+                                         /*MissingKind=*/0);
+    return;
+  }
+  case Builtin::BImemcpy:
+  case Builtin::BI__builtin_memcpy:
+    if (TheCall->getNumArgs() != 3)
+      return;
+    break;
+  default:
+    return;
+  }
+
+  const Expr *SrcArg = TheCall->getArg(1)->IgnoreParenCasts();
+  const Expr *SizeArg = TheCall->getArg(2)->IgnoreParenCasts();
+  const Expr *StrlenArg = getDirectStrlenCallArgument(SizeArg);
+  if (StrlenArg && areSameSimpleDeclRef(StrlenArg, SrcArg))
+    DiagnoseMissingNullTerminatorSpace(S, TheCall->getBeginLoc(), SizeArg,
+                                       /*MissingKind=*/1);
+}
+
 void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
                                                CallExpr *TheCall) {
   if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
@@ -4449,6 +4549,7 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
   CheckAbsoluteValueFunction(TheCall, FDecl);
   CheckMaxUnsignedZero(TheCall, FDecl);
   CheckInfNaNFunction(TheCall, FDecl);
+  CheckCStringNullTerminatorSizeArguments(*this, FDecl, TheCall);
 
   if (getLangOpts().ObjC)
     ObjC().DiagnoseCStringFormatDirectiveInCFAPI(FDecl, Args, NumArgs);
diff --git a/clang/test/Sema/warn-fortify-missing-null-terminator-space.c b/clang/test/Sema/warn-fortify-missing-null-terminator-space.c
new file mode 100644
index 0000000000000..2a79de4d93a7b
--- /dev/null
+++ b/clang/test/Sema/warn-fortify-missing-null-terminator-space.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -Wfortify-source -verify %s
+
+typedef __SIZE_TYPE__ size_t;
+
+void *malloc(size_t);
+void *memcpy(void *, const void *, size_t);
+size_t strlen(const char *);
+
+void direct_malloc_strlen(const char *src) {
+  char *p = malloc(strlen(src)); // expected-warning{{allocation size does not include space for null terminator; consider 'strlen(src) + 1'}}
+  char *ok = malloc(strlen(src) + 1);
+  (void)p;
+  (void)ok;
+}
+
+void direct_memcpy_strlen(char *dst, const char *src) {
+  memcpy(dst, src, strlen(src)); // expected-warning{{copy size does not include space for null terminator; consider 'strlen(src) + 1'}}
+  memcpy(dst, src, strlen(src) + 1);
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/190447