[Sema] Handle UTF-8 invalid format string specifiers

[bruno]

Summary

Improve invalid format string specifier handling by printing out invalid specifiers characters with \x, \u and \U. Previously clang would print gargabe whenever the character is unprintable.

Example, before:

NSLog(@"%\u25B9", 3); => warning: invalid conversion specifier ' [-Wformat-invalid-specifier]

after:

NSLog(@"%\u25B9", 3); => warning: invalid conversion specifier '\u25b9' [-Wformat-invalid-specifier]

[bruno]

Ping!

[bruno]

Ping :-)

[rsmith]

This patch builds a length-1 ConversionSpecifier but includes the complete code point in the length of the overall format specifier, which is inconsistent. Please either treat the trailing bytes as part of the ConversionSpecifier or revert the changes to ParsePrintfSpecifier and handle this entirely within HandleInvalidConversionSpecifier.

Does the same problem exist when parsing scanf specifiers?

lib/Analysis/PrintfFormatString.cpp

320

in -> is

322

The interpretation of a format string by printf should not depend on the locale, so our parsing of a format string should not either.

[bruno]

In D18296#384766, @rsmith wrote:

This patch builds a length-1 ConversionSpecifier but includes the complete code point in the length of the overall format specifier, which is inconsistent. Please either treat the trailing bytes as part of the ConversionSpecifier or revert the changes to ParsePrintfSpecifier and handle this entirely within HandleInvalidConversionSpecifier.

Ok, gonna handle this entirely within HandleInvalidConversionSpecifier then.

Does the same problem exist when parsing scanf specifiers?

Yes, I missed that, will update accordingly.

lib/Analysis/PrintfFormatString.cpp

322

llvm::sys::locale::isPrint does not actually do any locale specific check (maybe it should be moved elsewhere for better consitency?):

bool isPrint(int UCS) {
#if LLVM_ON_WIN32
  // Restrict characters that we'll try to print to the lower part of ASCII
  // except for the control characters (0x20 - 0x7E). In general one can not
  // reliably output code points U+0080 and higher using narrow character C/C++
  // output functions in Windows, because the meaning of the upper 128 codes is
  // determined by the active code page in the console.
  return ' ' <= UCS && UCS <= '~';
#else
  return llvm::sys::unicode::isPrintable(UCS);
#endif
}

This logic is needed anyway though. Suggestions?

[bruno]

Update after Richard's review.

• Handle scanf
• Properly update ConversionSpecifier

[rsmith]

lib/Analysis/FormatString.cpp

276

How about using getNumBytesForUTF8(FirstByte) != 1 here?

278

Perhaps only check &SB, &SB + Len -- it doesn't seem problematic if there's some non-UTF8 data after the specifier.

[bruno]

Thanks Richard. Applied your last comments and committed in r264752

include/clang/Analysis/Analyses/FormatString.h

bool consumesDataArgument() const {
     }
   }
 
   Kind getKind() const { return kind; }
   void setKind(Kind k) { kind = k; }
   unsigned getLength() const {
     return EndScanList ? EndScanList - Position : 1;
   }
+  void setEndScanList(const char *pos) { EndScanList = pos; }
 
   bool isIntArg() const { return (kind >= IntArgBeg && kind <= IntArgEnd) ||
     kind == FreeBSDrArg || kind == FreeBSDyArg; }
   bool isUIntArg() const { return kind >= UIntArgBeg && kind <= UIntArgEnd; }
   bool isAnyIntArg() const { return kind >= IntArgBeg && kind <= UIntArgEnd; }
   const char *toString() const;
 
   bool isPrintfKind() const { return IsPrintf; }
PrintfConversionSpecifier()
     : ConversionSpecifier(true, nullptr, InvalidSpecifier) {}
 
   PrintfConversionSpecifier(const char *pos, Kind k)
     : ConversionSpecifier(true, pos, k) {}
 
   bool isObjCArg() const { return kind >= ObjCBeg && kind <= ObjCEnd; }
   bool isDoubleArg() const { return kind >= DoubleArgBeg &&
                                     kind <= DoubleArgEnd; }
-  unsigned getLength() const {
-      // Conversion specifiers currently only are represented by
-      // single characters, but we be flexible.
-    return 1;
-  }
 
   static bool classof(const analyze_format_string::ConversionSpecifier *CS) {
     return CS->isPrintfKind();
   }
 };
 
 using analyze_format_string::ArgType;
 using analyze_format_string::LengthModifier;
class ScanfConversionSpecifier :
     public analyze_format_string::ConversionSpecifier  {
 public:
   ScanfConversionSpecifier()
     : ConversionSpecifier(false, nullptr, InvalidSpecifier) {}
 
   ScanfConversionSpecifier(const char *pos, Kind k)
     : ConversionSpecifier(false, pos, k) {}
 
-  void setEndScanList(const char *pos) { EndScanList = pos; }
-
   static bool classof(const analyze_format_string::ConversionSpecifier *CS) {
     return !CS->isPrintfKind();
   }
 };
 
 using analyze_format_string::ArgType;
 using analyze_format_string::LengthModifier;
 using analyze_format_string::OptionalAmount;

lib/Analysis/FormatString.cpp

 // Shared details for processing format strings of printf and scanf
 // (and friends).
 //
 //===----------------------------------------------------------------------===//
 
 #include "FormatStringParsing.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetInfo.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Locale.h"
 
 using clang::analyze_format_string::ArgType;
 using clang::analyze_format_string::FormatStringHandler;
 using clang::analyze_format_string::FormatSpecifier;
 using clang::analyze_format_string::LengthModifier;
 using clang::analyze_format_string::OptionalAmount;
 using clang::analyze_format_string::PositionContext;
 using clang::analyze_format_string::ConversionSpecifier;
switch (*I) {
     case 'w':
       lmKind = LengthModifier::AsWide; ++I; break;
   }
   LengthModifier lm(lmPosition, lmKind);
   FS.setLengthModifier(lm);
   return true;
 }
 
+bool clang::analyze_format_string::ParseUTF8InvalidSpecifier(
+    const char *SpecifierBegin, const char *FmtStrEnd, unsigned &Len) {
+  if (SpecifierBegin + 1 >= FmtStrEnd)
+    return false;
+
+  const UTF8 *SB = reinterpret_cast<const UTF8 *>(SpecifierBegin + 1);
+  const UTF8 *SE = reinterpret_cast<const UTF8 *>(FmtStrEnd);
+  const char FirstByte = *SB;
+
+  // If the specifier is non-printable, it could be the first byte of a
+  // UTF-8 sequence. If that's the case, adjust the length accordingly.
+  if (llvm::sys::locale::isPrint(FirstByte))
+    return false;
+  if (!isLegalUTF8String(&SB, SE))
+    return false;
+
+  Len = getNumBytesForUTF8(FirstByte) + 1;
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // Methods on ArgType.
 //===----------------------------------------------------------------------===//
 
 clang::analyze_format_string::ArgType::MatchKind
 ArgType::matchesType(ASTContext &C, QualType argTy) const {
   if (Ptr) {
     // It has to be a pointer.

lib/Analysis/FormatStringParsing.h

 bool ParseArgPosition(FormatStringHandler &H,
                       FormatSpecifier &CS, const char *Start,
                       const char *&Beg, const char *E);
 
 /// Returns true if a LengthModifier was parsed and installed in the
 /// FormatSpecifier& argument, and false otherwise.
 bool ParseLengthModifier(FormatSpecifier &FS, const char *&Beg, const char *E,
                          const LangOptions &LO, bool IsScanf = false);
-  
+/// Returns true if the invalid specifier in \p SpecifierBegin is a UTF-8
+/// string; check that it won't go further than \p FmtStrEnd and write
+/// up the total size in \p Len.
+bool ParseUTF8InvalidSpecifier(const char *SpecifierBegin,
+                               const char *FmtStrEnd, unsigned &Len);
+
 template <typename T> class SpecifierResult {
   T FS;
   const char *Start;
   bool Stop;
 public:
   SpecifierResult(bool stop = false)
   : Start(nullptr), Stop(stop) {}
   SpecifierResult(const char *start,

lib/Analysis/PrintfFormatString.cpp

static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
   if (CS.consumesDataArgument() && !FS.usesPositionalArg())
     FS.setArgIndex(argIndex++);
   // FreeBSD kernel specific.
   if (k == ConversionSpecifier::FreeBSDbArg ||
       k == ConversionSpecifier::FreeBSDDArg)
     argIndex++;
 
   if (k == ConversionSpecifier::InvalidSpecifier) {
+    unsigned Len = I - Start;
+    if (ParseUTF8InvalidSpecifier(Start, E, Len)) {
+      CS.setEndScanList(Start + Len);
+      FS.setConversionSpecifier(CS);
+    }
     // Assume the conversion takes one argument.
-    return !H.HandleInvalidPrintfConversionSpecifier(FS, Start, I - Start);
+    return !H.HandleInvalidPrintfConversionSpecifier(FS, Start, Len);
   }
   return PrintfSpecifierResult(Start, FS);
 }
 
 bool clang::analyze_format_string::ParsePrintfString(FormatStringHandler &H,
                                                      const char *I,
                                                      const char *E,
                                                      const LangOptions &LO,
                                                      const TargetInfo &Target,

lib/Analysis/ScanfFormatString.cpp

 // FIXME: Much of this is copy-paste from ParsePrintfSpecifier.
 // We can possibly refactor.
 static ScanfSpecifierResult ParseScanfSpecifier(FormatStringHandler &H,
                                                 const char *&Beg,
                                                 const char *E,
                                                 unsigned &argIndex,
                                                 const LangOptions &LO,
                                                 const TargetInfo &Target) {
-  
+  using namespace clang::analyze_format_string;
   using namespace clang::analyze_scanf;
   const char *I = Beg;
   const char *Start = nullptr;
   UpdateOnReturn <const char*> UpdateBeg(Beg, I);
 
     // Look for a '%' character that indicates the start of a format specifier.
   for ( ; I != E ; ++I) {
     char c = *I;
static ScanfSpecifierResult ParseScanfSpecifier(FormatStringHandler &H,
   }
   FS.setConversionSpecifier(CS);
   if (CS.consumesDataArgument() && !FS.getSuppressAssignment()
       && !FS.usesPositionalArg())
     FS.setArgIndex(argIndex++);
   
   // FIXME: '%' and '*' doesn't make sense.  Issue a warning.
   // FIXME: 'ConsumedSoFar' and '*' doesn't make sense.
-  
   if (k == ScanfConversionSpecifier::InvalidSpecifier) {
+    unsigned Len = I - Beg;
+    if (ParseUTF8InvalidSpecifier(Beg, E, Len)) {
+      CS.setEndScanList(Beg + Len);
+      FS.setConversionSpecifier(CS);
+    }
     // Assume the conversion takes one argument.
-    return !H.HandleInvalidScanfConversionSpecifier(FS, Beg, I - Beg);
+    return !H.HandleInvalidScanfConversionSpecifier(FS, Beg, Len);
   }
   return ScanfSpecifierResult(Start, FS);
 }
 
 ArgType ScanfSpecifier::getArgType(ASTContext &Ctx) const {
   const ScanfConversionSpecifier &CS = getConversionSpecifier();
 
   if (!CS.consumesDataArgument())

lib/Sema/SemaChecking.cpp

 #include "clang/Lex/Lexer.h" // TODO: Extract static functions to fix layering.
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/Locale.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/raw_ostream.h"
 #include <limits>
 
 using namespace clang;
 using namespace sema;
 
 SourceLocation Sema::getLocationOfStringLiteralByte(const StringLiteral *SL,
CheckFormatHandler::HandleInvalidConversionSpecifier(unsigned argIndex,
   else {
     // If argIndex exceeds the number of data arguments we
     // don't issue a warning because that is just a cascade of warnings (and
     // they may have intended '%%' anyway). We don't want to continue processing
     // the format string after this point, however, as we will like just get
     // gibberish when trying to match arguments.
     keepGoing = false;
   }
-  
-  EmitFormatDiagnostic(S.PDiag(diag::warn_format_invalid_conversion)
-                         << StringRef(csStart, csLen),
-                       Loc, /*IsStringLocation*/true,
-                       getSpecifierRange(startSpec, specifierLen));
-  
+  StringRef Specifier(csStart, csLen);
+
+  // If the specifier in non-printable, it could be the first byte of a UTF-8
+  // sequence. In that case, print the UTF-8 code point. If not, print the byte
+  // hex value.
+  std::string CodePointStr;
+  if (!llvm::sys::locale::isPrint(*csStart)) {
+    UTF32 CodePoint;
+    const UTF8 **B = reinterpret_cast<const UTF8 **>(&csStart);
+    const UTF8 *E =
+        reinterpret_cast<const UTF8 *>(csStart + csLen);
+    ConversionResult Result =
+        llvm::convertUTF8Sequence(B, E, &CodePoint, strictConversion);
+
+    if (Result != conversionOK) {
+      unsigned char FirstChar = *csStart;
+      CodePoint = (UTF32)FirstChar;
+    }
+
+    llvm::raw_string_ostream OS(CodePointStr);
+    if (CodePoint < 256)
+      OS << "\\x" << llvm::format("%02x", CodePoint);
+    else if (CodePoint <= 0xFFFF)
+      OS << "\\u" << llvm::format("%04x", CodePoint);
+    else
+      OS << "\\U" << llvm::format("%08x", CodePoint);
+    OS.flush();
+    Specifier = CodePointStr;
+  }
+
+  EmitFormatDiagnostic(
+      S.PDiag(diag::warn_format_invalid_conversion) << Specifier, Loc,
+      /*IsStringLocation*/ true, getSpecifierRange(startSpec, specifierLen));
   return keepGoing;
 }
 
 void
 CheckFormatHandler::HandlePositionalNonpositionalArgs(SourceLocation Loc,
                                                       const char *startSpec,
                                                       unsigned specifierLen) {
   EmitFormatDiagnostic(

test/SemaObjC/format-strings-objc.m

void testObjCModifierFlags() {
   NSLog(@"%[tt]@", @"Foo"); // no-warning
   NSLog(@"%[tt]@ %s", @"Foo", "hello"); // no-warning
   NSLog(@"%s %[tt]@", "hello", @"Foo"); // no-warning
   NSLog(@"%[blark]@", @"Foo"); // expected-warning {{'blark' is not a valid object format flag}}
   NSLog(@"%2$[tt]@ %1$[tt]@", @"Foo", @"Bar"); // no-warning
   NSLog(@"%2$[tt]@ %1$[tt]s", @"Foo", @"Bar"); // expected-warning {{object format flags cannot be used with 's' conversion specifier}}
 }
 
+// Test Objective-C invalid no printable specifiers
+void testObjcInvalidNoPrintable(int *a) {
+  NSLog(@"%\u25B9", 3); // expected-warning {{invalid conversion specifier '\u25b9'}}
+  NSLog(@"%\xE2\x96\xB9", 3); // expected-warning {{invalid conversion specifier '\u25b9'}}
+  NSLog(@"%\U00010348", 42); // expected-warning {{invalid conversion specifier '\U00010348'}}
+  NSLog(@"%\xF0\x90\x8D\x88", 42); // expected-warning {{invalid conversion specifier '\U00010348'}}
+  NSLog(@"%\xe2", @"Foo"); // expected-warning {{input conversion stopped}} expected-warning {{invalid conversion specifier '\xe2'}}
+  scanf("%\u25B9", a); // expected-warning {{implicitly declaring library}} expected-note {{include the header}} expected-warning {{invalid conversion specifier '\u25b9'}}
+  scanf("%\xE2\x96\xB9", a); // expected-warning {{invalid conversion specifier '\u25b9'}}
+  scanf("%\U00010348", a); // expected-warning {{invalid conversion specifier '\U00010348'}}
+  scanf("%\xF0\x90\x8D\x88", a); // expected-warning {{invalid conversion specifier '\U00010348'}}
+  scanf("%\xe2", a); // expected-warning {{invalid conversion specifier '\xe2'}}
+}