Patch by Cristian Draghici:

Enhance the printf format string checking when using the format
specifier flags ' ', '0', '+' with the 'p' or 's' conversions (since
they are nonsensical and undefined).  This is similar to GCC's
checking.

Also warning when a precision is used with the 'p' conversin
specifier, since it has no meaning.

llvm-svn: 95869

clang/include/clang/Analysis/Analyses/PrintfFormatString.h

@@ -73,6 +73,10 @@ public:
   const char *getStart() const {
     return Position;
   }
+  
+  llvm::StringRef getCharacters() const {
+    return llvm::StringRef(getStart(), getLength());
+  }
 	
   bool consumesDataArgument() const {
     switch (kind) {
@@ -232,6 +236,7 @@ public:
   bool hasPlusPrefix() const { return (bool) HasPlusPrefix; }
   bool hasAlternativeForm() const { return (bool) HasAlternativeForm; }
   bool hasLeadingZeros() const { return (bool) HasLeadingZeroes; }  
+  bool hasSpacePrefix() const { return (bool) HasSpacePrefix; }
 };
 
 class FormatStringHandler {

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2514,7 +2514,13 @@ def warn_printf_asterisk_width_wrong_type : Warning<
 def warn_printf_asterisk_precision_wrong_type : Warning<
   "field precision should have type %0, but argument has type %1">,
   InGroup<Format>;
-
+def warn_printf_nonsensical_precision: Warning<
+  "precision used in '%0' conversion specifier (where it has no meaning)">,
+  InGroup<Format>;
+def warn_printf_nonsensical_flag: Warning<
+  "flag '%0' results in undefined behavior in '%1' conversion specifier">,
+  InGroup<Format>;
+  
 // CHECK: returning address/reference of stack memory
 def warn_ret_stack_addr : Warning<
   "address of stack memory associated with local variable %0 returned">;

clang/lib/Sema/SemaChecking.cpp

@@ -1080,6 +1080,9 @@ private:
   bool HandleAmount(const analyze_printf::OptionalAmount &Amt,
                     unsigned MissingArgDiag, unsigned BadTypeDiag,
           const char *startSpecifier, unsigned specifierLen);
+  void HandleFlags(const analyze_printf::FormatSpecifier &FS,
+                   llvm::StringRef flag, llvm::StringRef cspec,
+                   const char *startSpecifier, unsigned specifierLen);
   
   bool MatchType(QualType A, QualType B, bool ignoreSign);
   
@@ -1175,6 +1178,16 @@ bool CheckPrintfHandler::MatchType(QualType A, QualType B, bool ignoreSign) {
   return false;  
 }
 
+void CheckPrintfHandler::HandleFlags(const analyze_printf::FormatSpecifier &FS,
+                                     llvm::StringRef flag,
+                                     llvm::StringRef cspec,
+                                     const char *startSpecifier,
+                                     unsigned specifierLen) {
+  const analyze_printf::ConversionSpecifier &CS = FS.getConversionSpecifier();
+  S.Diag(getLocationOfByte(CS.getStart()), diag::warn_printf_nonsensical_flag)
+    << flag << cspec << getFormatSpecifierRange(startSpecifier, specifierLen);
+}
+
 bool
 CheckPrintfHandler::HandleAmount(const analyze_printf::OptionalAmount &Amt,
                                  unsigned MissingArgDiag,
@@ -1214,7 +1227,8 @@ CheckPrintfHandler::HandleAmount(const analyze_printf::OptionalAmount &Amt,
 }
 
 bool
-CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier &FS,
+CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
+                                            &FS,
                                           const char *startSpecifier,
                                           unsigned specifierLen) {
 
@@ -1262,7 +1276,25 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
     // Continue checking the other format specifiers.
     return true;
   }
-  
+
+  if (CS.getKind() == ConversionSpecifier::VoidPtrArg) {
+    if (FS.getPrecision().getHowSpecified() != OptionalAmount::NotSpecified)
+      S.Diag(getLocationOfByte(CS.getStart()), 
+             diag::warn_printf_nonsensical_precision)
+        << CS.getCharacters()
+        << getFormatSpecifierRange(startSpecifier, specifierLen);
+  }
+  if (CS.getKind() == ConversionSpecifier::VoidPtrArg || 
+      CS.getKind() == ConversionSpecifier::CStrArg) {    
+    // FIXME: Instead of using "0", "+", etc., eventually get them from
+    // the FormatSpecifier.
+    if (FS.hasLeadingZeros())
+      HandleFlags(FS, "0", CS.getCharacters(), startSpecifier, specifierLen);
+    if (FS.hasPlusPrefix())
+      HandleFlags(FS, "+", CS.getCharacters(), startSpecifier, specifierLen);
+    if (FS.hasSpacePrefix())
+      HandleFlags(FS, " ", CS.getCharacters(), startSpecifier, specifierLen);
+  }  
   
   // The remaining checks depend on the data arguments.
   if (HasVAListArg)

clang/test/Sema/format-strings.c

@@ -171,6 +171,18 @@ void test10(int x, float f, int i, long long lli) {
   printf("%f\n", (long double) 1.0); // expected-warning{{conversion specifies type 'double' but the argument has type 'long double'}}
 } 
 
+void test11(void *p, char *s) {
+  printf("%p", p); // no-warning
+  printf("%.4p", p); // expected-warning{{precision used in 'p' conversion specifier (where it has no meaning)}}
+  printf("%+p", p); // expected-warning{{flag '+' results in undefined behavior in 'p' conversion specifier}}
+  printf("% p", p); // expected-warning{{flag ' ' results in undefined behavior in 'p' conversion specifier}}
+  printf("%0p", p); // expected-warning{{flag '0' results in undefined behavior in 'p' conversion specifier}}
+  printf("%s", s); // no-warning
+  printf("%+s", p); // expected-warning{{flag '+' results in undefined behavior in 's' conversion specifier}}
+  printf("% s", p); // expected-warning{{flag ' ' results in undefined behavior in 's' conversion specifier}}
+  printf("%0s", p); // expected-warning{{flag '0' results in undefined behavior in 's' conversion specifier}}
+}
+
 typedef struct __aslclient *aslclient;
 typedef struct __aslmsg *aslmsg;
 int asl_log(aslclient asl, aslmsg msg, int level, const char *format, ...) __attribute__((__format__ (__printf__, 4, 5)));