Objective-C. Under a special flag, -Wcstring-format-directive,

off by default, issue a warning if %s directive is used in
certain CF/NS formatting APIs, to assist user in deprecating
use of such %s in these APIs. rdar://18182443

llvm-svn: 217467

clang/include/clang/AST/Decl.h

@@ -289,6 +289,8 @@ public:
     return const_cast<NamedDecl*>(this)->getMostRecentDecl();
   }
 
+  ObjCStringFormatFamily getObjCFStringFormattingFamily() const;
+
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K >= firstNamed && K <= lastNamed; }
 };

clang/include/clang/Analysis/Analyses/FormatString.h

@@ -647,6 +647,9 @@ public:
 bool ParsePrintfString(FormatStringHandler &H,
                        const char *beg, const char *end, const LangOptions &LO,
                        const TargetInfo &Target);
+  
+bool ParseFormatStringHasSArg(const char *beg, const char *end, const LangOptions &LO,
+                              const TargetInfo &Target);
 
 bool ParseScanfString(FormatStringHandler &H,
                       const char *beg, const char *end, const LangOptions &LO,

clang/include/clang/Basic/DiagnosticGroups.td

@@ -350,6 +350,7 @@ def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
 def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
 def MethodDuplicate : DiagGroup<"duplicate-method-match">;
+def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool     : DiagGroup<"switch-bool">;
 def SwitchEnum     : DiagGroup<"switch-enum">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -719,6 +719,11 @@ def err_duplicate_method_decl : Error<"duplicate declaration of method %0">;
 def warn_duplicate_method_decl : 
   Warning<"multiple declarations of method %0 found and ignored">, 
   InGroup<MethodDuplicate>, DefaultIgnore;
+def warn_objc_cdirective_format_string :
+  Warning<"using %0 directive in %select{NSString|CFString}1 "
+          "which is being passed as a formatting argument to the formatting "
+          "%select{method|CFfunction}2">,
+  InGroup<ObjCCStringFormat>, DefaultIgnore;
 def err_objc_var_decl_inclass : 
     Error<"cannot declare variable inside @interface or @protocol">;
 def error_missing_method_context : Error<

clang/include/clang/Basic/IdentifierTable.h

@@ -589,6 +589,12 @@ enum ObjCInstanceTypeFamily {
   OIT_ReturnsSelf
 };
 
+enum ObjCStringFormatFamily {
+  SFF_None,
+  SFF_NSString,
+  SFF_CFString
+};
+
 /// \brief Smart pointer class that efficiently represents Objective-C method
 /// names.
 ///
@@ -634,6 +640,8 @@ class Selector {
   }
 
   static ObjCMethodFamily getMethodFamilyImpl(Selector sel);
+  
+  static ObjCStringFormatFamily getStringFormatFamilyImpl(Selector sel);
 
 public:
   friend class SelectorTable; // only the SelectorTable can create these
@@ -704,7 +712,11 @@ public:
   ObjCMethodFamily getMethodFamily() const {
     return getMethodFamilyImpl(*this);
   }
-
+  
+  ObjCStringFormatFamily getStringFormatFamily() const {
+    return getStringFormatFamilyImpl(*this);
+  }
+  
   static Selector getEmptyMarker() {
     return Selector(uintptr_t(-1));
   }

clang/include/clang/Sema/Sema.h

@@ -8403,6 +8403,8 @@ public:
                          FormatStringType Type, bool inFunctionCall,
                          VariadicCallType CallType,
                          llvm::SmallBitVector &CheckedVarArgs);
+  
+  bool FormatStringHasSArg(const StringLiteral *FExpr);
 
 private:
   bool CheckFormatArguments(const FormatAttr *Format,

clang/lib/AST/Decl.cpp

@@ -1001,6 +1001,19 @@ bool NamedDecl::isLinkageValid() const {
          getCachedLinkage();
 }
 
+ObjCStringFormatFamily NamedDecl::getObjCFStringFormattingFamily() const {
+  StringRef name = getName();
+  if (name.empty()) return SFF_None;
+  
+  if (name.front() == 'C')
+    if (name == "CFStringCreateWithFormat" ||
+        name == "CFStringCreateWithFormatAndArguments" ||
+        name == "CFStringAppendFormat" ||
+        name == "CFStringAppendFormatAndArguments")
+      return SFF_CFString;
+  return SFF_None;
+}
+
 Linkage NamedDecl::getLinkageInternal() const {
   // We don't care about visibility here, so ask for the cheapest
   // possible visibility analysis.

clang/lib/Analysis/PrintfFormatString.cpp

@@ -54,7 +54,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
                                                   const char *E,
                                                   unsigned &argIndex,
                                                   const LangOptions &LO,
-                                                  const TargetInfo &Target) {
+                                                  const TargetInfo &Target,
+                                                  bool Warn) {
 
   using namespace clang::analyze_format_string;
   using namespace clang::analyze_printf;
@@ -83,7 +84,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
 
   if (I == E) {
     // No more characters left?
-    H.HandleIncompleteSpecifier(Start, E - Start);
+    if (Warn)
+      H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
@@ -93,7 +95,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
 
   if (I == E) {
     // No more characters left?
-    H.HandleIncompleteSpecifier(Start, E - Start);
+    if (Warn)
+      H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
@@ -118,7 +121,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
 
   if (I == E) {
     // No more characters left?
-    H.HandleIncompleteSpecifier(Start, E - Start);
+    if (Warn)
+      H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
@@ -129,7 +133,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
 
   if (I == E) {
     // No more characters left?
-    H.HandleIncompleteSpecifier(Start, E - Start);
+    if (Warn)
+      H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
@@ -137,7 +142,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
   if (*I == '.') {
     ++I;
     if (I == E) {
-      H.HandleIncompleteSpecifier(Start, E - Start);
+      if (Warn)
+        H.HandleIncompleteSpecifier(Start, E - Start);
       return true;
     }
 
@@ -147,7 +153,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
 
     if (I == E) {
       // No more characters left?
-      H.HandleIncompleteSpecifier(Start, E - Start);
+      if (Warn)
+        H.HandleIncompleteSpecifier(Start, E - Start);
       return true;
     }
   }
@@ -155,7 +162,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
   // Look for the length modifier.
   if (ParseLengthModifier(FS, I, E, LO) && I == E) {
     // No more characters left?
-    H.HandleIncompleteSpecifier(Start, E - Start);
+    if (Warn)
+      H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
@@ -239,7 +247,7 @@ bool clang::analyze_format_string::ParsePrintfString(FormatStringHandler &H,
   // Keep looking for a format specifier until we have exhausted the string.
   while (I != E) {
     const PrintfSpecifierResult &FSR = ParsePrintfSpecifier(H, I, E, argIndex,
-                                                            LO, Target);
+                                                            LO, Target, true);
     // Did a fail-stop error of any kind occur when parsing the specifier?
     // If so, don't do any more processing.
     if (FSR.shouldStop())
@@ -257,6 +265,34 @@ bool clang::analyze_format_string::ParsePrintfString(FormatStringHandler &H,
   return false;
 }
 
+bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I,
+                                                            const char *E,
+                                                            const LangOptions &LO,
+                                                            const TargetInfo &Target) {
+  
+  unsigned argIndex = 0;
+  
+  // Keep looking for a %s format specifier until we have exhausted the string.
+  FormatStringHandler H;
+  while (I != E) {
+    const PrintfSpecifierResult &FSR = ParsePrintfSpecifier(H, I, E, argIndex,
+                                                            LO, Target, false);
+    // Did a fail-stop error of any kind occur when parsing the specifier?
+    // If so, don't do any more processing.
+    if (FSR.shouldStop())
+      return false;
+    // Did we exhaust the string or encounter an error that
+    // we can recover from?
+    if (!FSR.hasValue())
+      continue;
+    const analyze_printf::PrintfSpecifier &FS = FSR.getValue();
+    // Return true if this a %s format specifier.
+    if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::Kind::sArg)
+      return true;
+  }
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Methods on PrintfSpecifier.
 //===----------------------------------------------------------------------===//

clang/lib/Basic/IdentifierTable.cpp

@@ -487,6 +487,33 @@ ObjCInstanceTypeFamily Selector::getInstTypeMethodFamily(Selector sel) {
   return OIT_None;
 }
 
+ObjCStringFormatFamily Selector::getStringFormatFamilyImpl(Selector sel) {
+  IdentifierInfo *first = sel.getIdentifierInfoForSlot(0);
+  if (!first) return SFF_None;
+  
+  StringRef name = first->getName();
+  
+  switch (name.front()) {
+    case 'a':
+      if (name == "appendFormat") return SFF_NSString;
+      break;
+      
+    case 'i':
+      if (name == "initWithFormat") return SFF_NSString;
+      break;
+      
+    case 'l':
+      if (name == "localizedStringWithFormat") return SFF_NSString;
+      break;
+      
+    case 's':
+      if (name == "stringByAppendingFormat" ||
+          name == "stringWithFormat") return SFF_NSString;
+      break;
+  }
+  return SFF_None;
+}
+
 namespace {
   struct SelectorTableImpl {
     llvm::FoldingSet<MultiKeywordSelector> Table;

clang/lib/Sema/SemaChecking.cpp

@@ -767,6 +767,37 @@ static void CheckNonNullArgument(Sema &S,
     S.Diag(CallSiteLoc, diag::warn_null_arg) << ArgExpr->getSourceRange();
 }
 
+/// \brief Diagnose use of %s directive in an NSString which is being passed
+/// as formatting string to formatting method.
+static void
+DiagnoseCStringFormatDirectiveInCFAPI(Sema &S,
+                                        const NamedDecl *FDecl,
+                                        Expr **Args,
+                                        unsigned NumArgs) {
+  if (NumArgs < 3)
+    return;
+  ObjCStringFormatFamily SFFamily = FDecl->getObjCFStringFormattingFamily();
+  if (SFFamily == ObjCStringFormatFamily::SFF_CFString) {
+    const Expr *FormatExpr = Args[2];
+    if (const CStyleCastExpr *CSCE = dyn_cast<CStyleCastExpr>(FormatExpr))
+      FormatExpr = CSCE->getSubExpr();
+    const StringLiteral *FormatString;
+    if (const ObjCStringLiteral *OSL =
+        dyn_cast<ObjCStringLiteral>(FormatExpr->IgnoreParenImpCasts()))
+      FormatString = OSL->getString();
+    else
+      FormatString = dyn_cast<StringLiteral>(FormatExpr->IgnoreParenImpCasts());
+    if (!FormatString)
+      return;
+    if (S.FormatStringHasSArg(FormatString)) {
+      S.Diag(FormatExpr->getExprLoc(), diag::warn_objc_cdirective_format_string)
+        << "%s" << 1 << 1;
+        S.Diag(FDecl->getLocation(), diag::note_entity_declared_at)
+          << FDecl->getDeclName();
+    }
+  }
+}
+
 static void CheckNonNullArguments(Sema &S,
                                   const NamedDecl *FDecl,
                                   ArrayRef<const Expr *> Args,
@@ -899,6 +930,8 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
     return false;
 
   CheckAbsoluteValueFunction(TheCall, FDecl, FnInfo);
+  
+  DiagnoseCStringFormatDirectiveInCFAPI(*this, FDecl, Args, NumArgs);
 
   unsigned CMId = FDecl->getMemoryFunctionKind();
   if (CMId == 0)
@@ -3744,6 +3777,20 @@ void Sema::CheckFormatString(const StringLiteral *FExpr,
   } // TODO: handle other formats
 }
 
+bool Sema::FormatStringHasSArg(const StringLiteral *FExpr) {
+  // Str - The format string.  NOTE: this is NOT null-terminated!
+  StringRef StrRef = FExpr->getString();
+  const char *Str = StrRef.data();
+  // Account for cases where the string literal is truncated in a declaration.
+  const ConstantArrayType *T = Context.getAsConstantArrayType(FExpr->getType());
+  assert(T && "String literal not of constant array type!");
+  size_t TypeSize = T->getSize().getZExtValue();
+  size_t StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size());
+  return analyze_format_string::ParseFormatStringHasSArg(Str, Str + StrLen,
+                                                         getLangOpts(),
+                                                         Context.getTargetInfo());
+}
+
 //===--- CHECK: Warn on use of wrong absolute value function. -------------===//
 
 // Returns the related absolute value function that is larger, of 0 if one

clang/lib/Sema/SemaExprObjC.cpp

@@ -2114,6 +2114,32 @@ static void checkCocoaAPI(Sema &S, const ObjCMessageExpr *Msg) {
                      edit::rewriteObjCRedundantCallWithLiteral);
 }
 
+/// \brief Diagnose use of %s directive in an NSString which is being passed
+/// as formatting string to formatting method.
+static void
+DiagnoseCStringFormatDirectiveInObjCAPI(Sema &S,
+                                        ObjCMethodDecl *Method,
+                                        Selector Sel,
+                                        Expr **Args, unsigned NumArgs) {
+  if (NumArgs == 0)
+    return;
+  ObjCStringFormatFamily SFFamily = Sel.getStringFormatFamily();
+  if (SFFamily == ObjCStringFormatFamily::SFF_NSString) {
+    Expr *FormatExpr = Args[0];
+    if (ObjCStringLiteral *OSL =
+        dyn_cast<ObjCStringLiteral>(FormatExpr->IgnoreParenImpCasts())) {
+      StringLiteral *FormatString = OSL->getString();
+      if (S.FormatStringHasSArg(FormatString)) {
+        S.Diag(FormatExpr->getExprLoc(), diag::warn_objc_cdirective_format_string)
+        << "%s" << 0 << 0;
+        if (Method)
+          S.Diag(Method->getLocation(), diag::note_method_declared_at)
+          << Method->getDeclName();
+      }
+    }
+  }
+}
+
 /// \brief Build an Objective-C class message expression.
 ///
 /// This routine takes care of both normal class messages and
@@ -2258,7 +2284,9 @@ ExprResult Sema::BuildClassMessage(TypeSourceInfo *ReceiverTypeInfo,
       }
     }
   }
-
+  
+  DiagnoseCStringFormatDirectiveInObjCAPI(*this, Method, Sel, Args, NumArgs);
+  
   // Construct the appropriate ObjCMessageExpr.
   ObjCMessageExpr *Result;
   if (SuperLoc.isValid())
@@ -2743,6 +2771,8 @@ ExprResult Sema::BuildInstanceMessage(Expr *Receiver,
     }
   }
 
+  DiagnoseCStringFormatDirectiveInObjCAPI(*this, Method, Sel, Args, NumArgs);
+  
   // Construct the appropriate ObjCMessageExpr instance.
   ObjCMessageExpr *Result;
   if (SuperLoc.isValid())

clang/test/SemaObjC/format-cstrings-warning.m

@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -Wcstring-format-directive -verify -fsyntax-only %s
+// rdar://18182443
+
+typedef __builtin_va_list __darwin_va_list;
+typedef __builtin_va_list va_list;
+
+@interface NSString @end
+
+@interface NSString (NSStringExtensionMethods)
+- (NSString *)stringByAppendingFormat:(NSString *)format, ... __attribute__((format(__NSString__, 1, 2)));
+- (instancetype)initWithFormat:(NSString *)format, ... __attribute__((format(__NSString__, 1, 2))); // expected-note {{method 'initWithFormat:' declared here}}
+- (instancetype)initWithFormat:(NSString *)format arguments:(va_list)argList __attribute__((format(__NSString__, 1, 0)));
+- (instancetype)initWithFormat:(NSString *)format locale:(id)locale, ... __attribute__((format(__NSString__, 1, 3)));
+- (instancetype)initWithFormat:(NSString *)format locale:(id)locale arguments:(va_list)argList __attribute__((format(__NSString__, 1, 0)));
++ (instancetype)stringWithFormat:(NSString *)format, ... __attribute__((format(__NSString__, 1, 2))); // expected-note {{method 'stringWithFormat:' declared here}}
++ (instancetype)localizedStringWithFormat:(NSString *)format, ... __attribute__((format(__NSString__, 1, 2)));
+@end
+
+@interface NSMutableString : NSString
+@end
+
+@interface NSMutableString (NSMutableStringExtensionMethods)
+
+- (void)appendFormat:(NSString *)format, ... __attribute__((format(__NSString__, 1, 2)));
+
+@end
+
+NSString *ns(NSString *pns) {
+  [pns initWithFormat: @"Number %d length %c name %s", 1, 'a', "something"]; // expected-warning {{using %s directive in NSString which is being passed as a formatting argument to the formatting method}}
+  return [NSString stringWithFormat : @"Hello%s", " There"]; // expected-warning {{using %s directive in NSString which is being passed as a formatting argument to the formatting method}}
+}
+
+
+typedef const struct __CFString * CFStringRef;
+typedef struct __CFString * CFMutableStringRef;
+typedef const struct __CFAllocator * CFAllocatorRef;
+
+
+typedef const struct __CFDictionary * CFDictionaryRef;
+
+
+extern
+CFStringRef CFStringCreateWithFormat(CFAllocatorRef alloc, CFDictionaryRef formatOptions, CFStringRef format, ...) __attribute__((format(CFString, 3, 4)));
+
+extern
+CFStringRef CFStringCreateWithFormatAndArguments(CFAllocatorRef alloc, CFDictionaryRef formatOptions, CFStringRef format, va_list arguments) __attribute__((format(CFString, 3, 0))); // expected-note {{'CFStringCreateWithFormatAndArguments' declared here}}
+
+extern
+void CFStringAppendFormat(CFMutableStringRef theString, CFDictionaryRef formatOptions, CFStringRef format, ...) __attribute__((format(CFString, 3, 4)));
+
+extern
+void CFStringAppendFormatAndArguments(CFMutableStringRef theString, CFDictionaryRef formatOptions, CFStringRef format, va_list arguments) __attribute__((format(CFString, 3, 0))); // expected-note {{'CFStringAppendFormatAndArguments' declared here}}
+
+void foo(va_list argList) {
+  CFAllocatorRef alloc;
+  CFStringCreateWithFormatAndArguments (alloc, 0, (CFStringRef)@"%s\n", argList); // expected-warning {{using %s directive in CFString which is being passed as a formatting argument to the formatting CFfunction}}
+  CFStringAppendFormatAndArguments ((CFMutableStringRef)@"AAAA", 0, (CFStringRef)"Hello %s there %d\n", argList); // expected-warning {{using %s directive in CFString which is being passed as a formatting argument to the formatting CFfunction}}
+  CFStringCreateWithFormatAndArguments (alloc, 0, (CFStringRef)@"%c\n", argList);
+  CFStringAppendFormatAndArguments ((CFMutableStringRef)@"AAAA", 0, (CFStringRef)"%d\n", argList);
+}