Fix two bugs in format-string checking:

(1) Do not assume the data arguments start after the format string
(2) Do not use the fact that a function is variadic to treat it like a va_list printf function

Fixes PR 6697.

llvm-svn: 99480

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2594,6 +2594,9 @@ def warn_printf_missing_format_string : Warning<
 def warn_printf_conversion_argument_type_mismatch : Warning<
   "conversion specifies type %0 but the argument has type %1">,
   InGroup<Format>;
+def warn_printf_positional_arg_exceeds_data_args : Warning <
+  "data argument position '%0' exceeds the number of data arguments (%1)">,
+  InGroup<Format>;
 def warn_printf_zero_positional_specifier : Warning<
   "position arguments in format strings start counting at 1 (not 0)">,
   InGroup<Format>;

clang/lib/Sema/SemaChecking.cpp

@@ -222,11 +222,6 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) {
   if (const FormatAttr *Format = FDecl->getAttr<FormatAttr>()) {
     if (CheckablePrintfAttr(Format, TheCall)) {
       bool HasVAListArg = Format->getFirstArg() == 0;
-      if (!HasVAListArg) {
-        if (const FunctionProtoType *Proto
-            = FDecl->getType()->getAs<FunctionProtoType>())
-          HasVAListArg = !Proto->isVariadic();
-      }
       CheckPrintfArguments(TheCall, HasVAListArg, Format->getFormatIdx() - 1,
                            HasVAListArg ? 0 : Format->getFirstArg() - 1);
     }
@@ -257,12 +252,6 @@ bool Sema::CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall) {
     return false;
 
   bool HasVAListArg = Format->getFirstArg() == 0;
-  if (!HasVAListArg) {
-    const FunctionType *FT =
-      Ty->getAs<BlockPointerType>()->getPointeeType()->getAs<FunctionType>();
-    if (const FunctionProtoType *Proto = dyn_cast<FunctionProtoType>(FT))
-      HasVAListArg = !Proto->isVariadic();
-  }
   CheckPrintfArguments(TheCall, HasVAListArg, Format->getFormatIdx() - 1,
                        HasVAListArg ? 0 : Format->getFirstArg() - 1);
 
@@ -1045,6 +1034,7 @@ class CheckPrintfHandler : public analyze_printf::FormatStringHandler {
   Sema &S;
   const StringLiteral *FExpr;
   const Expr *OrigFormatExpr;
+  const unsigned FirstDataArg;
   const unsigned NumDataArgs;
   const bool IsObjCLiteral;
   const char *Beg; // Start of format string.
@@ -1056,11 +1046,12 @@ class CheckPrintfHandler : public analyze_printf::FormatStringHandler {
   bool atFirstArg;
 public:
   CheckPrintfHandler(Sema &s, const StringLiteral *fexpr,
-                     const Expr *origFormatExpr,
+                     const Expr *origFormatExpr, unsigned firstDataArg,
                      unsigned numDataArgs, bool isObjCLiteral,
                      const char *beg, bool hasVAListArg,
                      const CallExpr *theCall, unsigned formatIdx)
     : S(s), FExpr(fexpr), OrigFormatExpr(origFormatExpr),
+      FirstDataArg(firstDataArg),
       NumDataArgs(numDataArgs),
       IsObjCLiteral(isObjCLiteral), Beg(beg),
       HasVAListArg(hasVAListArg),
@@ -1183,11 +1174,9 @@ void CheckPrintfHandler::HandleNullChar(const char *nullCharacter) {
 }
 
 const Expr *CheckPrintfHandler::getDataArg(unsigned i) const {
-  return TheCall->getArg(FormatIdx + i + 1);
+  return TheCall->getArg(FirstDataArg + i);
 }
 
-
-
 void CheckPrintfHandler::HandleFlags(const analyze_printf::FormatSpecifier &FS,
                                      llvm::StringRef flag,
                                      llvm::StringRef cspec,
@@ -1329,9 +1318,18 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
     return true;
 
   if (argIndex >= NumDataArgs) {
-    S.Diag(getLocationOfByte(CS.getStart()),
+    if (FS.usesPositionalArg())  {
-           diag::warn_printf_insufficient_data_args)
+      S.Diag(getLocationOfByte(CS.getStart()),
-      << getFormatSpecifierRange(startSpecifier, specifierLen);
+             diag::warn_printf_positional_arg_exceeds_data_args)
+        << (argIndex+1) << NumDataArgs
+        << getFormatSpecifierRange(startSpecifier, specifierLen);
+    }
+    else {
+      S.Diag(getLocationOfByte(CS.getStart()),
+             diag::warn_printf_insufficient_data_args)
+        << getFormatSpecifierRange(startSpecifier, specifierLen);
+    }
+
     // Don't do any more checking.
     return false;
   }
@@ -1400,7 +1398,7 @@ void Sema::CheckPrintfString(const StringLiteral *FExpr,
     return;
   }
 
-  CheckPrintfHandler H(*this, FExpr, OrigFormatExpr,
+  CheckPrintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg,
                        TheCall->getNumArgs() - firstDataArg,
                        isa<ObjCStringLiteral>(OrigFormatExpr), Str,
                        HasVAListArg, TheCall, format_idx);

clang/test/Sema/format-strings.c

@@ -238,3 +238,16 @@ void test_positional_arguments() {
   printf("%2$*8$d", (int) 2, (int) 3); // expected-warning{{specified field width is missing a matching 'int' argument}}
 }
 
+// PR 6697 - Handle format strings where the data argument is not adjacent to the format string
+void myprintf_PR_6697(const char *format, int x, ...) __attribute__((__format__(printf,1, 3)));
+void test_pr_6697() {
+  myprintf_PR_6697("%s\n", 1, "foo"); // no-warning
+  myprintf_PR_6697("%s\n", 1, (int)0); // expected-warning{{conversion specifies type 'char *' but the argument has type 'int'}}
+  // FIXME: Not everything should clearly support positional arguments,
+  // but we need a way to identify those cases.
+  myprintf_PR_6697("%1$s\n", 1, "foo"); // no-warning
+  myprintf_PR_6697("%2$s\n", 1, "foo"); // expected-warning{{data argument position '2' exceeds the number of data arguments (1)}}
+  myprintf_PR_6697("%18$s\n", 1, "foo"); // expected-warning{{data argument position '18' exceeds the number of data arguments (1)}}
+  myprintf_PR_6697("%1$s\n", 1, (int) 0); // expected-warning{{conversion specifies type 'char *' but the argument has type 'int'}}
+}
+