Add new -Wformat warning for invalid type passed to a non-literal format string

[rtrieu]

Summary

Currently, when the format string is a non-literal, the only warning emitted and checked for is the non-literal format string. This patch checks the arguments passed to printf/scanf functions and checks that they are valid for the format string. For instance, warning on user defined types or non-pointers to scanf functions. Additionally, if only one argument is present, warn if it is a va_list. For built-in functions, suggest the proper va_list function to fix it.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_value_init_for_array_type : Error<
   "array types cannot be value-initialized">;
 def warn_format_nonliteral_noargs : Warning<
   "format string is not a string literal (potentially insecure)">,
   InGroup<FormatSecurity>;
 def warn_format_nonliteral : Warning<
   "format string is not a string literal">,
   InGroup<FormatNonLiteral>, DefaultIgnore;
+def warn_va_list_format_string_argument : Warning<
+  "passing a va_list to a variadic function">, InGroup<Format>;
+def note_va_list_function : Note<
+  "did you mean to use function %0 which takes a va_list argument?">;
+def warn_invalid_format_argument : Warning<
+   "invalid argument of type %0 to %select{printf|scanf}1 format string">,
+   InGroup<Format>;
 
 def err_unexpected_interface : Error<
   "unexpected interface name %0: expected expression">;
 def err_ref_non_value : Error<"%0 does not refer to a value">;
 def err_ref_vm_type : Error<
   "cannot refer to declaration with a variably modified type inside block">;
 def err_ref_flexarray_type : Error<
   "cannot refer to declaration of structure variable with flexible array member "

include/clang/Sema/Sema.h

public:
   bool GetFormatNSStringIdx(const FormatAttr *Format, unsigned &Idx);
 
 private:
   bool CheckFormatArguments(const FormatAttr *Format,
                             ArrayRef<const Expr *> Args,
                             bool IsCXXMember,
                             VariadicCallType CallType,
                             SourceLocation Loc, SourceRange Range,
-                            llvm::SmallBitVector &CheckedVarArgs);
+                            llvm::SmallBitVector &CheckedVarArgs,
+                            FunctionDecl *FD);
   bool CheckFormatArguments(ArrayRef<const Expr *> Args,
                             bool HasVAListArg, unsigned format_idx,
                             unsigned firstDataArg, FormatStringType Type,
                             VariadicCallType CallType,
                             SourceLocation Loc, SourceRange range,
-                            llvm::SmallBitVector &CheckedVarArgs);
+                            llvm::SmallBitVector &CheckedVarArgs,
+                            FunctionDecl *FD);
 
   void CheckAbsoluteValueFunction(const CallExpr *Call,
                                   const FunctionDecl *FDecl,
                                   IdentifierInfo *FnInfo);
 
   void CheckMemaccessArguments(const CallExpr *Call,
                                unsigned BId,
                                IdentifierInfo *FnName);

lib/Sema/SemaChecking.cpp

void Sema::checkCall(NamedDecl *FDecl, ArrayRef<const Expr *> Args,
   // Printf and scanf checking.
   llvm::SmallBitVector CheckedVarArgs;
   if (FDecl) {
     for (const auto *I : FDecl->specific_attrs<FormatAttr>()) {
       // Only create vector if there are format attributes.
       CheckedVarArgs.resize(Args.size());
 
       CheckFormatArguments(I, Args, IsMemberFunction, CallType, Loc, Range,
-                           CheckedVarArgs);
+                           CheckedVarArgs, dyn_cast<FunctionDecl>(FDecl));
     }
   }
 
   // Refuse POD arguments that weren't caught by the format string
   // checks above.
   if (CallType != VariadicDoesNotApply) {
     for (unsigned ArgIdx = NumParams; ArgIdx < Args.size(); ++ArgIdx) {
       // Args[ArgIdx] can be null in malformed code.

 /// CheckFormatArguments - Check calls to printf and scanf (and similar
 /// functions) for correct use of format strings.
 /// Returns true if a format string has been fully checked.
 bool Sema::CheckFormatArguments(const FormatAttr *Format,
                                 ArrayRef<const Expr *> Args,
                                 bool IsCXXMember,
                                 VariadicCallType CallType,
                                 SourceLocation Loc, SourceRange Range,
-                                llvm::SmallBitVector &CheckedVarArgs) {
+                                llvm::SmallBitVector &CheckedVarArgs,
+                                FunctionDecl *FD) {
   FormatStringInfo FSI;
   if (getFormatStringInfo(Format, IsCXXMember, &FSI))
     return CheckFormatArguments(Args, FSI.HasVAListArg, FSI.FormatIdx,
                                 FSI.FirstDataArg, GetFormatStringType(Format),
-                                CallType, Loc, Range, CheckedVarArgs);
+                                CallType, Loc, Range, CheckedVarArgs, FD);
+  return false;
+}
+
+/// Given a builtin vararg function, return the equivalent va_list function, or
+/// 0 otherwise.
+static unsigned GetVAListFunction(unsigned BuiltinID) {
+  switch (BuiltinID) {
+    default:
+      return 0;
+    case Builtin::BIprintf:
+      return Builtin::BIvprintf;
+    case Builtin::BIfprintf:
+      return Builtin::BIvfprintf;
+    case Builtin::BIsnprintf:
+      return Builtin::BIvsnprintf;
+    case Builtin::BIsprintf:
+      return Builtin::BIvsprintf;
+    case Builtin::BIscanf:
+      return Builtin::BIvscanf;
+    case Builtin::BIfscanf:
+      return Builtin::BIvfscanf;
+    case Builtin::BIsscanf:
+      return Builtin::BIvsscanf;
+    case Builtin::BINSLog:
+      return Builtin::BINSLogv;
+  }
+}
+
+/// Emit a diagnostic with a fix-it hint to replace the range with builtin
+/// function, qualifying it with "::" if necessary.
+static void EmitBuiltinFunctionHints(Sema &SemaRef, unsigned BuiltinID,
+                                     SourceRange Range, unsigned DiagID) {
+  std::string ReplacementName = SemaRef.Context.BuiltinInfo.GetName(BuiltinID);
+  // Look up builtin function in TU scope.
+  DeclarationName Name(&SemaRef.Context.Idents.get(ReplacementName));
+  LookupResult Result(SemaRef, Name, Range.getBegin(), Sema::LookupAnyName);
+  SemaRef.LookupName(Result, SemaRef.TUScope);
+
+  // Skip notes if multiple results found in lookup.
+  if (!Result.empty() && !Result.isSingleResult())
+    return;
+
+  FunctionDecl *FD = 0;
+  bool FoundFunction = Result.isSingleResult();
+  // When one result is found, see if it is the correct function.
+  if (Result.isSingleResult()) {
+    FD = dyn_cast<FunctionDecl>(Result.getFoundDecl());
+    if (!FD || FD->getBuiltinID() != BuiltinID)
+      return;
+  }
+
+  // Look for local name conflict, prepend "::" as necessary.
+  Result.clear();
+  SemaRef.LookupName(Result, SemaRef.getCurScope());
+
+  if (!FoundFunction) {
+    if (!Result.empty()) {
+      ReplacementName = "::" + ReplacementName;
+    }
+  } else { // FoundFunction
+    if (Result.isSingleResult()) {
+      if (Result.getFoundDecl() != FD) {
+        ReplacementName = "::" + ReplacementName;
+      }
+    } else if (!Result.empty()) {
+      ReplacementName = "::" + ReplacementName;
+    }
+  }
+
+  SemaRef.Diag(Range.getBegin(), DiagID)
+      << ReplacementName
+      << FixItHint::CreateReplacement(Range, ReplacementName);
+
+  if (!FoundFunction) {
+    SemaRef.Diag(Range.getBegin(), diag::note_include_header_or_declare)
+        << SemaRef.Context.BuiltinInfo.getHeaderName(BuiltinID)
+        << SemaRef.Context.BuiltinInfo.GetName(BuiltinID);
+  }
+}
+
+/// Check to see if Ty is a va_list.  Since a va_list can be an array, also
+/// check if Ty is a pointer to an array element, which can happen if Ty
+/// is from a parameter.
+static bool IsVaListType(ASTContext &Context, QualType Ty) {
+  QualType VaListType = Context.getBuiltinVaListType();
+  if (Context.hasSameType(Ty, VaListType))
+    return true;
+  if (VaListType->isArrayType() && Ty->isPointerType())
+    return Context.hasSameType(
+        VaListType->getAsArrayTypeUnsafe()->getElementType(),
+        Ty->getPointeeType());
   return false;
 }
 
 bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
                                 bool HasVAListArg, unsigned format_idx,
                                 unsigned firstDataArg, FormatStringType Type,
                                 VariadicCallType CallType,
                                 SourceLocation Loc, SourceRange Range,
-                                llvm::SmallBitVector &CheckedVarArgs) {
+                                llvm::SmallBitVector &CheckedVarArgs,
+                                FunctionDecl *FD) {
   // CHECK: printf/scanf-like function is called with no format string.
   if (format_idx >= Args.size()) {
     Diag(Loc, diag::warn_missing_format_string) << Range;
     return false;
   }
 
   const Expr *OrigFormatExpr = Args[format_idx]->IgnoreParenCasts();
 
bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
   // diag when using the NSLocalizedString and CFCopyLocalizedString macros
   // which are usually used in place of NS and CF string literals.
   if (Type == FST_NSString &&
       SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
     return false;
 
   // If there are no arguments specified, warn with -Wformat-security, otherwise
   // warn only with -Wformat-nonliteral.
-  if (Args.size() == firstDataArg)
+  if (Args.size() == firstDataArg) {
     Diag(Args[format_idx]->getLocStart(),
          diag::warn_format_nonliteral_noargs)
       << OrigFormatExpr->getSourceRange();
-  else
-    Diag(Args[format_idx]->getLocStart(),
-         diag::warn_format_nonliteral)
-           << OrigFormatExpr->getSourceRange();
+    return false;
+  }
+
+  Diag(Args[format_idx]->getLocStart(), diag::warn_format_nonliteral)
+
+  // Even with a non-literal format string, some checking on arguments is
+  // possible.
+  if (firstDataArg == 0)
+    return false;
+
+  if (firstDataArg + 1 == Args.size()) {
+    const Expr *Arg = Args[firstDataArg]->IgnoreImpCasts();
+    // Warn if there is exactly one variadic argument and that argument is a
+    // va_list type.  If possible, suggest the the va_list equivalent function.
+    if (IsVaListType(Context, Arg->getType())) {
+      Diag(Arg->getLocStart(), diag::warn_va_list_format_string_argument);
+
+      if (!FD)
+        return false;
+      const unsigned BuiltinID = GetVAListFunction(FD->getBuiltinID());
+      if (BuiltinID == 0)
+        return false;
+
+      EmitBuiltinFunctionHints(*this, BuiltinID, Range,
+                               diag::note_va_list_function);
+      return false;
+    }
+  }
+
+  if (Type == FST_Printf) {
+    for (unsigned i = firstDataArg, e = Args.size(); i < e; ++i) {
+      const Expr *Arg = Args[i];
+      QualType T = Arg->getType();
+      // Non-POD arguments are checked by -Wvarargs
+      if (!T.isPODType(Context))
+        continue;
+      if (T->isIntegerType() || T->isFloatingType())
+        continue;
+      if (T->isPointerType())
+        continue;
+
+      Diag(Arg->getLocStart(), diag::warn_invalid_format_argument)
+          << Arg->getType() << Arg->getSourceRange() << 0 /*printf*/;
+    }
+    return false;
+  }
+
+  if (Type == FST_Scanf) {
+    for (unsigned i = firstDataArg, e = Args.size(); i < e; ++i) {
+      const Expr *Arg = Args[i];
+      QualType T = Arg->getType();
+      // Non-POD arguments are checked by -Wvarargs
+      if (!T.isPODType(Context))
+        continue;
+      if (T->isPointerType()) {
+        QualType Pointee = T->getPointeeType();
+        if (!Pointee.isConstQualified())
+          if (Pointee->isIntegerType() || Pointee->isFloatingType())
+            continue;
+      }
+      Diag(Arg->getLocStart(), diag::warn_invalid_format_argument)
+          << Arg->getType() << Arg->getSourceRange() << 1 /*scanf*/;
+    }
+    return false;
+  }
+
   return false;
 }
 
 namespace {
 class CheckFormatHandler : public analyze_format_string::FormatStringHandler {
 protected:
   Sema &S;
   const StringLiteral *FExpr;

test/SemaCXX/format-strings-invalid-args.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wformat
+typedef __SIZE_TYPE__ size_t;
+typedef __builtin_va_list va_list;
+
+typedef struct _FILE FILE;
+
+extern "C" {
+
+int printf(const char *, ...);
+int snprintf(char *, size_t, const char *, ...);
+int sprintf(char *, const char *, ...);
+int asprintf(char **, const char *, ...);
+
+int vprintf(const char *, va_list);
+int vsnprintf(char *, size_t, const char *, va_list);
+int vsprintf(char *, const char *, va_list);
+int vasprintf(char **, const char *, va_list);
+
+
+int fprintf(FILE *, const char *, ...);
+
+int scanf(const char*, ...);
+int fscanf(FILE*, const char*, ...);
+int sscanf(const char*,  const char*, ...);
+
+}
+
+void test_builtins(const char *str, va_list vlist, FILE *file, char *out) {
+  printf(str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vprintf which takes a va_list argument?}}
+  snprintf(out, 0, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vsnprintf which takes a va_list argument?}}
+  sprintf(out, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vsprintf which takes a va_list argument?}}
+  asprintf(&out, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+
+  fprintf(file, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vfprintf which takes a va_list argument?}}
+  // expected-note@-3{{include the header <stdio.h> or explicitly provide a declaration for 'vfprintf'}}
+  scanf(str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vscanf which takes a va_list argument?}}
+  // expected-note@-3{{include the header <stdio.h> or explicitly provide a declaration for 'vscanf'}}
+  fscanf(file, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vfscanf which takes a va_list argument?}}
+  // expected-note@-3{{include the header <stdio.h> or explicitly provide a declaration for 'vfscanf'}}
+  sscanf(out, str, vlist);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  // expected-note@-2{{did you mean to use function vsscanf which takes a va_list argument?}}
+  // expected-note@-3{{include the header <stdio.h> or explicitly provide a declaration for 'vsscanf'}}
+}
+
+
+void print(const char*, ...) __attribute__((__format__(__printf__, 1, 2)));
+void scan(const char*, ...) __attribute__((__format__(__scanf__, 1, 2)));
+
+void test(const char* str) {
+  va_list list;
+  print(str, list);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+  scan(str, list);
+  // expected-warning@-1{{passing a va_list to a variadic function}}
+
+  int i;
+  long l;
+  long long ll;
+  unsigned int ui;
+  unsigned long ul;
+  unsigned long long ull;
+  const char * cc;
+  char * c;
+  char ac[5];
+  const char acc[5] = "Hi";
+  enum E { E1 } e;
+  struct X { } x;
+
+  print(str, i, l, ll, ui, ul, ull, cc, c, ac, acc, e, E1);
+  print(str, &i, &l, &ui, &ul, &ull, &cc, &c, &e);
+
+  print(str, x);
+  // expected-warning@-1{{invalid argument of type 'struct X' to printf format string}}
+
+  scan(str, &i, &l, &ui, &ul, &ull, &e);
+  scan(str, c, ac);
+
+  scan(str, i, l, ll, ui, ul, ull, e);
+  // expected-warning@-1{{invalid argument of type 'int' to scanf format string}}
+  // expected-warning@-2{{invalid argument of type 'long' to scanf format string}}
+  // expected-warning@-3{{invalid argument of type 'long long' to scanf format string}}
+  // expected-warning@-4{{invalid argument of type 'unsigned int' to scanf format string}}
+  // expected-warning@-5{{invalid argument of type 'unsigned long' to scanf format string}}
+  // expected-warning@-6{{invalid argument of type 'unsigned long long' to scanf format string}}
+  // expected-warning@-7{{invalid argument of type 'int' to scanf format string}}
+  scanf(str, cc, acc);
+  // expected-warning@-1{{invalid argument of type 'const char *' to scanf format string}}
+  // expected-warning@-2{{invalid argument of type 'const char *' to scanf format string}}
+  scanf(str, x, &x);
+  // expected-warning@-1{{invalid argument of type 'struct X' to scanf format string}}
+  // expected-warning@-2{{invalid argument of type 'struct X *' to scanf format string}}
+}