Bugprone unsafe format string

clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp

@@ -8,9 +8,7 @@
 
 #include "UnsafeFormatStringCheck.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/Lexer.h"
 #include "llvm/Support/ConvertUTF.h"
-#include "llvm/Support/raw_ostream.h"
 
 using namespace clang::ast_matchers;
 
@@ -21,13 +19,16 @@ UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name,
     : ClangTidyCheck(Name, Context) {}
 
 void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) {
-  // Match vulnerable format string functions
-  auto VulnerableFunctions = hasAnyName(
-      "sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf", "vfscanf",
-      "vsscanf", "wscanf", "fwscanf", "swscanf", "vwscanf", "vfwscanf", "vswscanf");
-
+  // Matches sprintf and scanf family functions in std namespace in C++ and
+  // globally in C.
+  auto VulnerableFunctions =
+      hasAnyName("sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf",
+                 "vfscanf", "vsscanf", "wscanf", "fwscanf", "swscanf",
+                 "vwscanf", "vfwscanf", "vswscanf");
   Finder->addMatcher(
-      callExpr(callee(functionDecl(VulnerableFunctions)),
+      callExpr(callee(functionDecl(VulnerableFunctions,
+                                   anyOf(isInStdNamespace(),
+                                         hasParent(translationUnitDecl())))),
                anyOf(hasArgument(0, stringLiteral().bind("format")),
                      hasArgument(1, stringLiteral().bind("format"))))
           .bind("call"),
@@ -54,85 +55,78 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) {
 
   const auto *Callee = cast<FunctionDecl>(Call->getCalleeDecl());
   StringRef FunctionName = Callee->getName();
-  
+
   bool IsScanfFamily = FunctionName.contains("scanf");
-  
+
   if (!hasUnboundedStringSpecifier(FormatString, IsScanfFamily))
     return;
 
   auto Diag = diag(Call->getBeginLoc(),
-                   IsScanfFamily 
+                   IsScanfFamily
                      ? "format specifier '%%s' without field width may cause buffer overflow; consider using '%%Ns' where N limits input length"
                      : "format specifier '%%s' without precision may cause buffer overflow; consider using '%%.Ns' where N limits output length")
               << Call->getSourceRange();
-
-  std::string SafeAlternative = getSafeAlternative(FunctionName);
-  if (!SafeAlternative.empty()) {
-    Diag << FixItHint::CreateInsertion(Call->getBeginLoc(),
-                                       "/* Consider using " + SafeAlternative + " */ ");
-  }
 }
 
-
-bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString, bool IsScanfFamily) {
+bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef Fmt,
+                                                          bool IsScanfFamily) {
   size_t Pos = 0;
-  while ((Pos = FormatString.find('%', Pos)) != StringRef::npos) {
-    if (Pos + 1 >= FormatString.size())
+  size_t N = Fmt.size();
+  while ((Pos = Fmt.find('%', Pos)) != StringRef::npos) {
+    if (Pos + 1 >= N)
       break;
 
     // Skip %%
-    if (FormatString[Pos + 1] == '%') {
+    if (Fmt[Pos + 1] == '%') {
       Pos += 2;
       continue;
     }
 
     size_t SpecPos = Pos + 1;
 
     // Skip flags
-    while (SpecPos < FormatString.size() &&
-           (FormatString[SpecPos] == '-' || FormatString[SpecPos] == '+' ||
-            FormatString[SpecPos] == ' ' || FormatString[SpecPos] == '#' ||
-            FormatString[SpecPos] == '0')) {
+    while (SpecPos < N &&
+           (Fmt[SpecPos] == '-' || Fmt[SpecPos] == '+' || Fmt[SpecPos] == ' ' ||
+            Fmt[SpecPos] == '#' || Fmt[SpecPos] == '0')) {
       SpecPos++;
     }
 
     // Check for field width
     bool HasFieldWidth = false;
-    if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') {
+    if (SpecPos < N && Fmt[SpecPos] == '*') {
       HasFieldWidth = true;
       SpecPos++;
     } else {
-      while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) {
+      while (SpecPos < N && isdigit(Fmt[SpecPos])) {
         HasFieldWidth = true;
         SpecPos++;
       }
     }
 
     // Check for precision
     bool HasPrecision = false;
-    if (SpecPos < FormatString.size() && FormatString[SpecPos] == '.') {
+    if (SpecPos < N && Fmt[SpecPos] == '.') {
       SpecPos++;
-      if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') {
+      if (SpecPos < N && Fmt[SpecPos] == '*') {
         HasPrecision = true;
         SpecPos++;
       } else {
-        while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) {
+        while (SpecPos < N && isdigit(Fmt[SpecPos])) {
           HasPrecision = true;
           SpecPos++;
         }
       }
     }
 
     // Skip length modifiers
-    while (SpecPos < FormatString.size() &&
-           (FormatString[SpecPos] == 'h' || FormatString[SpecPos] == 'l' ||
-            FormatString[SpecPos] == 'L' || FormatString[SpecPos] == 'z' ||
-            FormatString[SpecPos] == 'j' || FormatString[SpecPos] == 't')) {
+    while (SpecPos < N && (Fmt[SpecPos] == 'h' || Fmt[SpecPos] == 'l' ||
+                           Fmt[SpecPos] == 'L' || Fmt[SpecPos] == 'z' ||
+                           Fmt[SpecPos] == 'j' || Fmt[SpecPos] == 't')) {
       SpecPos++;
     }
 
     // Check for 's' specifier
-    if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's') {
+    if (SpecPos < N && Fmt[SpecPos] == 's') {
       if (IsScanfFamily) {
         // For scanf family, field width provides protection
         if (!HasFieldWidth) {
@@ -152,14 +146,4 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString
   return false;
 }
 
-std::string UnsafeFormatStringCheck::getSafeAlternative(StringRef FunctionName) {
-  if (FunctionName == "sprintf")
-    return "snprintf";
-  if (FunctionName == "vsprintf")
-    return "vsnprintf";
-  if (FunctionName.starts_with("scanf") || FunctionName.ends_with("scanf"))
-    return "add field width to %s specifiers";
-  return "";
-}
-
 } // namespace clang::tidy::bugprone

clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c

@@ -1,9 +1,36 @@
 // RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t
 
-#include <stdio.h>
-#include <wchar.h>
 #include <stdarg.h>
 
+typedef __SIZE_TYPE__  size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+typedef void *FILE;
+extern FILE *stdin;
+extern FILE *stderr;
+
+extern int fscanf ( FILE * stream, const char * format, ... );
+extern int scanf ( const char * format, ... );
+extern int sscanf ( const char * s, const char * format, ...);
+extern int vscanf( const char *restrict format, va_list vlist );
+extern int vfscanf ( FILE * stream, const char * format, va_list arg );
+
+extern int vsscanf( const char *restrict buffer, const char *restrict format, va_list vlist );
+extern int vwscanf( const wchar_t* format, va_list vlist );
+extern int vfwscanf( FILE* stream, const wchar_t* format, va_list vlist );
+extern int vswscanf( const wchar_t* buffer, const wchar_t* format, va_list vlist );
+extern int swscanf (const wchar_t* ws, const wchar_t* format, ...);
+extern int wscanf( const wchar_t *format, ... );
+extern int fwscanf( FILE *stream, const wchar_t *format, ... );
+
+extern int printf( const char*          format, ... );
+extern int sprintf( char* buffer, const char* format, ... );
+extern int vsprintf (char * s, const char * format, va_list arg );
+extern int vsnprintf (char * s, size_t n, const char * format, va_list arg );
+extern int fprintf( FILE*          stream, const char*          format, ... );
+extern int snprintf( char* restrict buffer, size_t bufsz,
+              const char* restrict format, ... );
+
+
 void test_sprintf() {
   char buffer[100];
   const char* input = "user input";

clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp

@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t
+
+namespace std {
+  int sprintf( char* buffer, const char* format, ... );
+}
+
+class TestClass{
+  int sprintf( char* buffer, const char* format, ... );
+  void test_sprintf() {
+    char buffer[100];
+    const char* input = "user input";
+    /* Negative: no warning for calling member functions */
+    sprintf(buffer, "%s", input);
+  }
+};
+
+void test_sprintf() {
+  char buffer[100];
+  const char* input = "user input";
+
+  /* Positive: unsafe %s without field width */
+  std::sprintf(buffer, "%s", input);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string]
+
+  /* Positive: field width doesn't prevent overflow in sprintf */
+  std::sprintf(buffer, "%99s", input);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string]
+
+  /* Positive: dynamic field width doesn't prevent overflow */
+  std::sprintf(buffer, "%*s", 10, input);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string]
+
+  /*Negative: precision limits string length */
+  std::sprintf(buffer, "%.99s", input);
+  /* no-warning */
+
+  /*Negative: precision with field width */
+  std::sprintf(buffer, "%1.99s", input);
+  /* no-warning */
+
+  /*Negative: dynamic precision */
+  std::sprintf(buffer, "%.*s", 99, input);
+  /* no-warning */
+
+  /*Negative: field width with dynamic precision */
+  std::sprintf(buffer, "%1.*s", 99, input);
+  /* no-warning */
+
+  /*Negative: dynamic field width with fixed precision */
+  std::sprintf(buffer, "%*.99s", 10, input);
+  /* no-warning */
+
+  /*Negative: dynamic field width and precision */
+  std::sprintf(buffer, "%*.*s", 10, 99, input);
+  /* no-warning */
+
+  /*Negative: other format specifiers are safe */
+  std::sprintf(buffer, "%d %f", 42, 3.14);
+  /* no-warning */
+}