From aa3855694ff45fc51bd71774c8bb15738b83ef16 Mon Sep 17 00:00:00 2001 From: Erik Pilkington Date: Wed, 14 Aug 2019 16:57:11 +0000 Subject: [PATCH] [Sema][ObjC] Fix a -Wformat false positive with localizedStringForKey Only honour format_arg attributes on -[NSBundle localizedStringForKey] when its argument has a format specifier in it, otherwise its likely to just be a key to fetch localized strings. Fixes rdar://23622446 Differential revision: https://reviews.llvm.org/D27165 llvm-svn: 368878 --- clang/include/clang/AST/FormatString.h | 6 +++ clang/include/clang/Basic/IdentifierTable.h | 6 +++ clang/lib/AST/PrintfFormatString.cpp | 17 +++++++++ clang/lib/Basic/IdentifierTable.cpp | 15 ++++++++ clang/lib/Sema/SemaChecking.cpp | 57 +++++++++++++++++++++-------- clang/test/SemaObjC/format-strings-objc.m | 42 ++++++++++++++++++++- 6 files changed, 127 insertions(+), 16 deletions(-) diff --git a/clang/include/clang/AST/FormatString.h b/clang/include/clang/AST/FormatString.h index 643fb82..aad77f5 100644 --- a/clang/include/clang/AST/FormatString.h +++ b/clang/include/clang/AST/FormatString.h @@ -748,6 +748,12 @@ bool ParseScanfString(FormatStringHandler &H, const char *beg, const char *end, const LangOptions &LO, const TargetInfo &Target); +/// Return true if the given string has at least one formatting specifier. +bool parseFormatStringHasFormattingSpecifiers(const char *Begin, + const char *End, + const LangOptions &LO, + const TargetInfo &Target); + } // end analyze_format_string namespace } // end clang namespace #endif diff --git a/clang/include/clang/Basic/IdentifierTable.h b/clang/include/clang/Basic/IdentifierTable.h index 465486e..37d7198 100644 --- a/clang/include/clang/Basic/IdentifierTable.h +++ b/clang/include/clang/Basic/IdentifierTable.h @@ -750,6 +750,12 @@ public: return getIdentifierInfoFlag() == ZeroArg; } + /// If this selector is the specific keyword selector described by Names. + bool isKeywordSelector(ArrayRef Names) const; + + /// If this selector is the specific unary selector described by Name. + bool isUnarySelector(StringRef Name) const; + unsigned getNumArgs() const; /// Retrieve the identifier at a given position in the selector. diff --git a/clang/lib/AST/PrintfFormatString.cpp b/clang/lib/AST/PrintfFormatString.cpp index 68f7829..bae60d4 100644 --- a/clang/lib/AST/PrintfFormatString.cpp +++ b/clang/lib/AST/PrintfFormatString.cpp @@ -463,6 +463,23 @@ bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I, return false; } +bool clang::analyze_format_string::parseFormatStringHasFormattingSpecifiers( + const char *Begin, const char *End, const LangOptions &LO, + const TargetInfo &Target) { + unsigned ArgIndex = 0; + // Keep looking for a formatting specifier until we have exhausted the string. + FormatStringHandler H; + while (Begin != End) { + const PrintfSpecifierResult &FSR = + ParsePrintfSpecifier(H, Begin, End, ArgIndex, LO, Target, false, false); + if (FSR.shouldStop()) + break; + if (FSR.hasValue()) + return true; + } + return false; +} + //===----------------------------------------------------------------------===// // Methods on PrintfSpecifier. //===----------------------------------------------------------------------===// diff --git a/clang/lib/Basic/IdentifierTable.cpp b/clang/lib/Basic/IdentifierTable.cpp index ca9c712..4aebea1 100644 --- a/clang/lib/Basic/IdentifierTable.cpp +++ b/clang/lib/Basic/IdentifierTable.cpp @@ -412,6 +412,21 @@ public: } // namespace clang. +bool Selector::isKeywordSelector(ArrayRef Names) const { + assert(!Names.empty() && "must have >= 1 selector slots"); + if (getNumArgs() != Names.size()) + return false; + for (unsigned I = 0, E = Names.size(); I != E; ++I) { + if (getNameForSlot(I) != Names[I]) + return false; + } + return true; +} + +bool Selector::isUnarySelector(StringRef Name) const { + return isUnarySelector() && getNameForSlot(0) == Name; +} + unsigned Selector::getNumArgs() const { unsigned IIF = getIdentifierInfoFlag(); if (IIF <= ZeroArg) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 3225dff..07db04e 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -6575,7 +6575,8 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr, bool inFunctionCall, Sema::VariadicCallType CallType, llvm::SmallBitVector &CheckedVarArgs, - UncoveredArgHandler &UncoveredArg); + UncoveredArgHandler &UncoveredArg, + bool IgnoreStringsWithoutSpecifiers); // Determine if an expression is a string literal or constant string. // If this function returns false on the arguments to a function expecting a @@ -6588,7 +6589,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, Sema::VariadicCallType CallType, bool InFunctionCall, llvm::SmallBitVector &CheckedVarArgs, UncoveredArgHandler &UncoveredArg, - llvm::APSInt Offset) { + llvm::APSInt Offset, + bool IgnoreStringsWithoutSpecifiers = false) { if (S.isConstantEvaluated()) return SLCT_NotALiteral; tryAgain: @@ -6639,17 +6641,17 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, Left = checkFormatStringExpr(S, C->getTrueExpr(), Args, HasVAListArg, format_idx, firstDataArg, Type, CallType, InFunctionCall, - CheckedVarArgs, UncoveredArg, Offset); + CheckedVarArgs, UncoveredArg, Offset, + IgnoreStringsWithoutSpecifiers); if (Left == SLCT_NotALiteral || !CheckRight) { return Left; } } - StringLiteralCheckType Right = - checkFormatStringExpr(S, C->getFalseExpr(), Args, - HasVAListArg, format_idx, firstDataArg, - Type, CallType, InFunctionCall, CheckedVarArgs, - UncoveredArg, Offset); + StringLiteralCheckType Right = checkFormatStringExpr( + S, C->getFalseExpr(), Args, HasVAListArg, format_idx, firstDataArg, + Type, CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset, + IgnoreStringsWithoutSpecifiers); return (CheckLeft && Left < Right) ? Left : Right; } @@ -6753,7 +6755,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, const Expr *Arg = CE->getArg(FA->getFormatIdx().getASTIndex()); StringLiteralCheckType Result = checkFormatStringExpr( S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type, - CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset); + CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset, + IgnoreStringsWithoutSpecifiers); if (IsFirst) { CommonResult = Result; IsFirst = false; @@ -6771,7 +6774,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, HasVAListArg, format_idx, firstDataArg, Type, CallType, InFunctionCall, CheckedVarArgs, - UncoveredArg, Offset); + UncoveredArg, Offset, + IgnoreStringsWithoutSpecifiers); } } } @@ -6780,12 +6784,28 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, } case Stmt::ObjCMessageExprClass: { const auto *ME = cast(E); - if (const auto *ND = ME->getMethodDecl()) { - if (const auto *FA = ND->getAttr()) { + if (const auto *MD = ME->getMethodDecl()) { + if (const auto *FA = MD->getAttr()) { + // As a special case heuristic, if we're using the method -[NSBundle + // localizedStringForKey:value:table:], ignore any key strings that lack + // format specifiers. The idea is that if the key doesn't have any + // format specifiers then its probably just a key to map to the + // localized strings. If it does have format specifiers though, then its + // likely that the text of the key is the format string in the + // programmer's language, and should be checked. + const ObjCInterfaceDecl *IFace; + if (MD->isInstanceMethod() && (IFace = MD->getClassInterface()) && + IFace->getIdentifier()->isStr("NSBundle") && + MD->getSelector().isKeywordSelector( + {"localizedStringForKey", "value", "table"})) { + IgnoreStringsWithoutSpecifiers = true; + } + const Expr *Arg = ME->getArg(FA->getFormatIdx().getASTIndex()); return checkFormatStringExpr( S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type, - CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset); + CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset, + IgnoreStringsWithoutSpecifiers); } } @@ -6809,7 +6829,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef Args, FormatStringLiteral FStr(StrE, Offset.sextOrTrunc(64).getSExtValue()); CheckFormatString(S, &FStr, E, Args, HasVAListArg, format_idx, firstDataArg, Type, InFunctionCall, CallType, - CheckedVarArgs, UncoveredArg); + CheckedVarArgs, UncoveredArg, + IgnoreStringsWithoutSpecifiers); return SLCT_CheckedLiteral; } @@ -8500,7 +8521,8 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr, bool inFunctionCall, Sema::VariadicCallType CallType, llvm::SmallBitVector &CheckedVarArgs, - UncoveredArgHandler &UncoveredArg) { + UncoveredArgHandler &UncoveredArg, + bool IgnoreStringsWithoutSpecifiers) { // CHECK: is the format string a wide literal? if (!FExpr->isAscii() && !FExpr->isUTF8()) { CheckFormatHandler::EmitFormatDiagnostic( @@ -8521,6 +8543,11 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr, size_t StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size()); const unsigned numDataArgs = Args.size() - firstDataArg; + if (IgnoreStringsWithoutSpecifiers && + !analyze_format_string::parseFormatStringHasFormattingSpecifiers( + Str, Str + StrLen, S.getLangOpts(), S.Context.getTargetInfo())) + return; + // Emit a warning if the string literal is truncated and does not contain an // embedded null character. if (TypeSize <= StrRef.size() && diff --git a/clang/test/SemaObjC/format-strings-objc.m b/clang/test/SemaObjC/format-strings-objc.m index 767d5ac..e5a1a82 100644 --- a/clang/test/SemaObjC/format-strings-objc.m +++ b/clang/test/SemaObjC/format-strings-objc.m @@ -25,7 +25,11 @@ typedef struct _NSZone NSZone; @protocol NSCoding - (void)encodeWithCoder:(NSCoder *)aCoder; @end @interface NSObject {} @end typedef float CGFloat; -@interface NSString : NSObject - (NSUInteger)length; @end +@interface NSString : NSObject +- (NSUInteger)length; ++(instancetype)stringWithFormat:(NSString *)fmt, ... + __attribute__((format(__NSString__, 1, 2))); +@end @interface NSSimpleCString : NSString {} @end @interface NSConstantString : NSSimpleCString @end extern void *_NSConstantStringClassReference; @@ -302,3 +306,39 @@ const char *rd23622446(const char *format) { } @end + +@interface NSBundle : NSObject +- (NSString *)localizedStringForKey:(NSString *)key + value:(nullable NSString *)value + table:(nullable NSString *)tableName + __attribute__((format_arg(1))); + +- (NSString *)someRandomMethod:(NSString *)key + value:(nullable NSString *)value + table:(nullable NSString *)tableName + __attribute__((format_arg(1))); +@end + +void useLocalizedStringForKey(NSBundle *bndl) { + [NSString stringWithFormat: + [bndl localizedStringForKey:@"%d" // expected-warning{{more '%' conversions than data arguments}} + value:0 + table:0]]; + // No warning, @"flerp" doesn't have a format specifier. + [NSString stringWithFormat: [bndl localizedStringForKey:@"flerp" value:0 table:0], 43, @"flarp"]; + + [NSString stringWithFormat: + [bndl localizedStringForKey:@"%f" + value:0 + table:0], 42]; // expected-warning{{format specifies type 'double' but the argument has type 'int'}} + + [NSString stringWithFormat: + [bndl someRandomMethod:@"%f" + value:0 + table:0], 42]; // expected-warning{{format specifies type 'double' but the argument has type 'int'}} + + [NSString stringWithFormat: + [bndl someRandomMethod:@"flerp" + value:0 + table:0], 42]; // expected-warning{{data argument not used by format string}} +} -- 2.7.4