add fix-its for format-security warnings

[bob.wilson]

Summary

The format-security warning is a special case of format-nonliteral that applies when there are no arguments besides the format string. In those cases, for printf and NSLog-style functions, there is an easy fix to provide a literal format string of "%s" (or @"%@" for Objective-C), with the nonliteral string as the argument. This patch teaches clang to provide fix-its for those cases.

[bcraig]

What about wprintf? Do we currently warn for wprintf(str)? If so, then the fixit probably needs to involve L"%ls".

[bob.wilson]

In D17941#369698, @bcraig wrote:

What about wprintf? Do we currently warn for wprintf(str)? If so, then the fixit probably needs to involve L"%ls".

Darwin does not mark wprintf functions with an attribute. Linux (at least the version I checked) has an attribute that is commented out, but it uses a distinct "wprintf" format type. Clang does not currently support that format type. If that is added in the future, you are right that the fix-it will need to be different.

[bcraig]

LGTM. I don't have a lot of authority over this code, so you may want to get a separate LGTM from someone else.

[bob.wilson]

Thanks Ben. Committed in r263299

lib/Sema/SemaChecking.cpp

   // format is either NSString or CFString. This is a hack to prevent
   // diag when using the NSLocalizedString and CFCopyLocalizedString macros
   // which are usually used in place of NS and CF string literals.
-  if (Type == FST_NSString &&
-      SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
+  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
+  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
     return false;
 
   // If there are no arguments specified, warn with -Wformat-security, otherwise
   // warn only with -Wformat-nonliteral.
-  if (Args.size() == firstDataArg)
-    Diag(Args[format_idx]->getLocStart(),
-         diag::warn_format_nonliteral_noargs)
+  if (Args.size() == firstDataArg) {
+    const SemaDiagnosticBuilder &D =
+      Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
+    switch (Type) {
+    default:
+      D << OrigFormatExpr->getSourceRange();
+      break;
+    case FST_Kprintf:
+    case FST_FreeBSDKPrintf:
+    case FST_Printf:
+      D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
+      break;
+    case FST_NSString:
+      D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
+      break;
+    }
+  } else {
+    Diag(FormatLoc, diag::warn_format_nonliteral)
       << OrigFormatExpr->getSourceRange();
-  else
-    Diag(Args[format_idx]->getLocStart(),
-         diag::warn_format_nonliteral)
-           << OrigFormatExpr->getSourceRange();
+  }
   return false;
 }
 

test/Sema/format-strings-fixit.c

 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
 
+extern const char *NonliteralString;
+
 void test() {
   // Basic types
   printf("%s", (int) 123);

   printf("%G", (long double) 42);
   printf("%a", (long double) 42);
   printf("%A", (long double) 42);
+
+  // nonliteral format
+  printf(NonliteralString);
 }
 
 int scanf(char const *, ...);

 // CHECK: printf("%LG", (long double) 42);
 // CHECK: printf("%La", (long double) 42);
 // CHECK: printf("%LA", (long double) 42);
+// CHECK: printf("%s", NonliteralString);
 
 // CHECK: scanf("%99s", str);
 // CHECK: scanf("%s", vstr);

test/SemaObjC/format-strings-objc-fixit.m

+// RUN: cp %s %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -pedantic -Wall -fixit %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -fsyntax-only -pedantic -Wall -Werror %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -E -o - %t | FileCheck %s
+
+typedef signed char BOOL;
+typedef unsigned int NSUInteger;
+typedef struct _NSZone NSZone;
+@class NSCoder, NSString, NSEnumerator;
+@protocol NSObject  - (BOOL)isEqual:(id)object; @end
+@protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
+@protocol NSMutableCopying  - (id)mutableCopyWithZone:(NSZone *)zone; @end
+@protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
+@interface NSObject <NSObject> {} @end
+@interface NSString : NSObject <NSCopying, NSMutableCopying, NSCoding>  - (NSUInteger)length; @end
+extern void NSLog(NSString *format, ...);
+
+/* This is a test of the various code modification hints that are
+   provided as part of warning or extension diagnostics. All of the
+   warnings will be fixed by -fixit, and the resulting file should
+   compile cleanly with -Werror -pedantic. */
+
+extern NSString *NonliteralString;
+
+void test() {
+  // nonliteral format
+  NSLog(NonliteralString);
+}
+
+// Validate the fixes.
+// CHECK: NSLog(@"%@", NonliteralString);