From d6d59f0664c7a2c73314117d2544f71d6eff86a4 Mon Sep 17 00:00:00 2001 From: Fariborz Jahanian Date: Tue, 9 Sep 2014 23:10:54 +0000 Subject: [PATCH] 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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@217467 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 2 + .../clang/Analysis/Analyses/FormatString.h | 3 + include/clang/Basic/DiagnosticGroups.td | 1 + include/clang/Basic/DiagnosticSemaKinds.td | 5 ++ include/clang/Basic/IdentifierTable.h | 14 ++++- include/clang/Sema/Sema.h | 2 + lib/AST/Decl.cpp | 13 ++++ lib/Analysis/PrintfFormatString.cpp | 54 ++++++++++++++--- lib/Basic/IdentifierTable.cpp | 27 +++++++++ lib/Sema/SemaChecking.cpp | 47 +++++++++++++++ lib/Sema/SemaExprObjC.cpp | 32 +++++++++- test/SemaObjC/format-cstrings-warning.m | 60 +++++++++++++++++++ 12 files changed, 249 insertions(+), 11 deletions(-) create mode 100644 test/SemaObjC/format-cstrings-warning.m diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index 95c95ea04e..3aff0f5424 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -289,6 +289,8 @@ public: return const_cast(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; } }; diff --git a/include/clang/Analysis/Analyses/FormatString.h b/include/clang/Analysis/Analyses/FormatString.h index d75194679c..174cce4f36 100644 --- a/include/clang/Analysis/Analyses/FormatString.h +++ b/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, diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index c9872d8b60..1ebe037ca3 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/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">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index e826578a15..7852dc78bd 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/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, 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, DefaultIgnore; def err_objc_var_decl_inclass : Error<"cannot declare variable inside @interface or @protocol">; def error_missing_method_context : Error< diff --git a/include/clang/Basic/IdentifierTable.h b/include/clang/Basic/IdentifierTable.h index e63e04f772..a6a174a41c 100644 --- a/include/clang/Basic/IdentifierTable.h +++ b/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)); } diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index de9b761152..d0a49596d1 100644 --- a/include/clang/Sema/Sema.h +++ b/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, diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 08a6490f14..4be4184f9b 100644 --- a/lib/AST/Decl.cpp +++ b/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. diff --git a/lib/Analysis/PrintfFormatString.cpp b/lib/Analysis/PrintfFormatString.cpp index c6453b6654..146635b887 100644 --- a/lib/Analysis/PrintfFormatString.cpp +++ b/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. //===----------------------------------------------------------------------===// diff --git a/lib/Basic/IdentifierTable.cpp b/lib/Basic/IdentifierTable.cpp index 01f60a9d69..a81369ad27 100644 --- a/lib/Basic/IdentifierTable.cpp +++ b/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 Table; diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 435b951b7c..0c3a3df1f1 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/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(FormatExpr)) + FormatExpr = CSCE->getSubExpr(); + const StringLiteral *FormatString; + if (const ObjCStringLiteral *OSL = + dyn_cast(FormatExpr->IgnoreParenImpCasts())) + FormatString = OSL->getString(); + else + FormatString = dyn_cast(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 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 diff --git a/lib/Sema/SemaExprObjC.cpp b/lib/Sema/SemaExprObjC.cpp index d4a2d76766..d1c7aa6a43 100644 --- a/lib/Sema/SemaExprObjC.cpp +++ b/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(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()) diff --git a/test/SemaObjC/format-cstrings-warning.m b/test/SemaObjC/format-cstrings-warning.m new file mode 100644 index 0000000000..dea9cbd476 --- /dev/null +++ b/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); +} -- 2.40.0