improve clang's static analysis of __builtin_sprintf_chk()

[samparker]

Summary

Issue: https://llvm.org/bugs/show_bug.cgi?id=21124

Patch to improve Clang's static analysis of
__builtin_sprintf_chk so that it will outperform the
analysis of g++.

[samparker]

Hi Guys,

Has anyone been able to look at this yet? Its been a while and I understand it was a blocker for building android with clang.

Cheers,
sam

[rtrieu]

Hi Sam,

I'm just seeing this now since Phab didn't email me when I was added as a reviewer. I will review the patch this week.

Richard

[samparker]

cheers!

[rtrieu]

These are comments about everything besides the actual checking code. I will review that next.

1. Double warnings are coming up because the format strings are being checked twice. Clang already knows that __builtin___sprintf_chk has a format string, so that gets checked with the old diagnostics. Instead of calling the checks directly, modify the existing path to carry extra information along.
   • Add an argument bool IsSprintf at the end of Sema::CheckFormatArguments
   • When Sema::CheckFormatArguments is called in Sema::checkCall, set that argument to true if FDecl is a FunctionDecl that is the function __builtin___sprintf_chk
   • In Sema::CheckFormatArguments, if IsSprintf is true, set the type to FST_Sprintf, otherwise, use the current type.

2. From testing, your patch looks limited in a few areas. The limitations of this warning should be documented with your CheckSprintfHandler class. For instance, non-specifiers text in the format string isn't counted. There's two ways to go from here:

3. See http://llvm.org/docs/Phabricator.html about how to add full context to your patches. Full context makes reviewing patches easier.

include/clang/Sema/Sema.h

8524–8527

Remove.

8569

Add comment that this is not a format string attribute, but extra checking on top of a printf format string.

lib/Sema/SemaChecking.cpp

483–485

Remove.

1323–1356

Remove.

3395

Don't make unrelated whitespace changes in your patch.

4091–4094

Do you care that the expression is exactly a DeclRefExpr? It doens't seem important to this warning and you can remove everything except the return statement.

4106

Remove isObjC parameter.

4113

Replace isObjC with /*isObjC=*/ false.

4124

What is this function indicating? It needs a better name, a comment, or both.

4136–4137

Why are APInt's used here instead of an integer type?

4250–4251

A new diagnostic message should be created with more information than the current overflow diagnostic, and added to the format warning group.

4512

Remove this line and remove this parameter from the constructor, since the type is known at this point.

4519

Replace with /*isFreeBSDKPrintf=*/false)

test/SemaCXX/sprintf-chk.cpp

1 ↗

(On Diff #25474)

There is nothing C++ about this. Move test to test/Sema/ directory.

13 ↗

(On Diff #25474)

Move the expected-warning to the next line and use the @-1 to declare the warning comes from the previous line.

line_generating_warning();
// expected-warning@-1 {{warning message}}

Also, make sure that at least one of the expected-warning checks the whole warning message. Right now, they all check truncated versions of the warning.

[rtrieu]

lib/Sema/SemaChecking.cpp

4168

This seems backwards. You using the type of expression to determine how to calculate the value. You should use the argument type along with a switch statement to handle all the different specifiers.

4172

Another problem of trying to match expressions is that there are plenty of strange cases, for instance, negative numbers are actually positive numbers wrapped in a unary negative expression. Also, constants won't show up, or 1+1. Instead, use the Evaluate* methods in Expr. If it is a compile time constant, then Evaulate* will provide the value.

4226–4228

This is kind of tricky. Pointers have an implementation defined printing style. We should make sure that this takes into account how different implementations print pointers.

4236

StorageRequired is the total size needed for the entire string, not just the space required for this specifier. A local size is needed so that the extra width is added on correctly.

[samparker]

Thanks for all the feedback and tips, I have hopefully addressed the comments from the first patch by:

• reorganised the patch so that it only uses the printf specifier expression checker,
• I have also removed the CheckSprintfHandler class and moved the functionality into CheckPrintfHandler,
• used a switch statement on ConversionSpecifier::Kind to check each specifier,
• used Expr functionality to evaluate constant integer expressions,
• added platform specific checks for pointer formatting,
• I have created a specific error message which reports the size needed.

cheers,

[samparker]

Hi Richard,

Have you had a chance to look at the revision?

Thanks,
sam

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_assume_side_effects : Warning<
   "the argument to %0 has side effects that will be discarded">,
   InGroup<DiagGroup<"assume">>;
 
 def warn_memcpy_chk_overflow : Warning<
   "%0 will always overflow destination buffer">,
   InGroup<DiagGroup<"builtin-memcpy-chk-size">>;
 
+def warn_sprintf_chk_overflow : Warning<
+  "call will always overflow destination buffer as it requires at least %0 "
+  "bytes whereas %1 are provided">,
+  InGroup<Format>;
+
 /// main()
 // static main() is not an error in C, just in C++.
 def warn_static_main : Warning<"'main' should not be declared static">,
     InGroup<Main>;
 def err_static_main : Error<"'main' is not allowed to be declared static">;
 def err_inline_main : Error<"'main' is not allowed to be declared inline">;
 def ext_variadic_main : ExtWarn<
   "'main' is not allowed to be declared variadic">, InGroup<Main>;

include/clang/Sema/Sema.h

enum ParserCompletionContext {
     /// category implementation
     PCC_ObjCImplementation,
     /// \brief Code completion occurs within the list of instance variables
     /// in an Objective-C interface, protocol, category, or implementation.
     PCC_ObjCInstanceVariableList,
     /// \brief Code completion occurs following one or more template
     /// headers.
     PCC_Template,
     /// \brief Code completion occurs following one or more template
     /// headers within a class.
     PCC_MemberTemplate,
     /// \brief Code completion occurs within an expression.
     PCC_Expression,
     /// \brief Code completion occurs within a statement, which may
     /// also be an expression or a declaration.
     PCC_Statement,
     /// \brief Code completion occurs at the beginning of the
     /// initialization statement (or expression) in a for loop.
     PCC_ForInit,
     /// \brief Code completion occurs within the condition of an if,
void CodeCompleteExpression(Scope *S,
                               const CodeCompleteExpressionData &Data);
   void CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
                                        SourceLocation OpLoc,
                                        bool IsArrow);
   void CodeCompletePostfixExpression(Scope *S, ExprResult LHS);
   void CodeCompleteTag(Scope *S, unsigned TagSpec);
   void CodeCompleteTypeQualifiers(DeclSpec &DS);
   void CodeCompleteCase(Scope *S);
   void CodeCompleteCall(Scope *S, Expr *Fn, ArrayRef<Expr *> Args);
   void CodeCompleteConstructor(Scope *S, QualType Type, SourceLocation Loc,
                                ArrayRef<Expr *> Args);
   void CodeCompleteInitializer(Scope *S, Decl *D);
   void CodeCompleteReturn(Scope *S);
   void CodeCompleteAfterIf(Scope *S);
   void CodeCompleteAssignmentRHS(Scope *S, Expr *LHS);
 
   void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
private:
   bool SemaBuiltinARMSpecialReg(unsigned BuiltinID, CallExpr *TheCall,
                                 int ArgNum, unsigned ExpectedFieldNum,
                                 bool AllowName);
   bool SemaBuiltinCpuSupports(CallExpr *TheCall);
 public:
   enum FormatStringType {
     FST_Scanf,
     FST_Printf,
+    FST_Sprintf,    // extra check on top of Printf
     FST_NSString,
     FST_Strftime,
     FST_Strfmon,
     FST_Kprintf,
     FST_FreeBSDKPrintf,
     FST_OSTrace,
     FST_Unknown
   };
public:
   bool GetFormatNSStringIdx(const FormatAttr *Format, unsigned &Idx);
 
 private:
   bool CheckFormatArguments(const FormatAttr *Format,
                             ArrayRef<const Expr *> Args,
                             bool IsCXXMember,
                             VariadicCallType CallType,
                             SourceLocation Loc, SourceRange Range,
-                            llvm::SmallBitVector &CheckedVarArgs);
+                            llvm::SmallBitVector &CheckedVarArgs,
+                            bool isSprintf);
   bool CheckFormatArguments(ArrayRef<const Expr *> Args,
                             bool HasVAListArg, unsigned format_idx,
                             unsigned firstDataArg, FormatStringType Type,
                             VariadicCallType CallType,
                             SourceLocation Loc, SourceRange range,
-                            llvm::SmallBitVector &CheckedVarArgs);
+                            llvm::SmallBitVector &CheckedVarArgs,
+                            bool isSprintf);
 
   void CheckAbsoluteValueFunction(const CallExpr *Call,
                                   const FunctionDecl *FDecl,
                                   IdentifierInfo *FnInfo);
 
   void CheckMemaccessArguments(const CallExpr *Call,
                                unsigned BId,
                                IdentifierInfo *FnName);

lib/Sema/SemaChecking.cpp

#include "clang/Basic/Builtins.def"
   case Builtin::BI__builtin___memset_chk:
   case Builtin::BI__builtin___strlcat_chk:
   case Builtin::BI__builtin___strlcpy_chk:
   case Builtin::BI__builtin___strncat_chk:
   case Builtin::BI__builtin___strncpy_chk:
   case Builtin::BI__builtin___stpncpy_chk:
     SemaBuiltinMemChkCall(*this, FDecl, TheCall, 2, 3);
     break;
   case Builtin::BI__builtin___memccpy_chk:
     SemaBuiltinMemChkCall(*this, FDecl, TheCall, 3, 4);
     break;
   case Builtin::BI__builtin___snprintf_chk:
   case Builtin::BI__builtin___vsnprintf_chk:
     SemaBuiltinMemChkCall(*this, FDecl, TheCall, 1, 3);
     break;
 
   case Builtin::BI__builtin_call_with_static_chain:
     if (SemaBuiltinCallWithStaticChain(*this, TheCall))
       return ExprError();
void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
                      VariadicCallType CallType) {
   // FIXME: We should check as much as we can in the template definition.
   if (CurContext->isDependentContext())
     return;
 
   // Printf and scanf checking.
   llvm::SmallBitVector CheckedVarArgs;
   if (FDecl) {
+    bool isSprintf = false;
+    if (auto *CastFDecl = dyn_cast<FunctionDecl>(FDecl))
+      if (CastFDecl->getBuiltinID() == Builtin::BI__builtin___sprintf_chk)
+        isSprintf = true;
     for (const auto *I : FDecl->specific_attrs<FormatAttr>()) {
       // Only create vector if there are format attributes.
       CheckedVarArgs.resize(Args.size());
 
       CheckFormatArguments(I, Args, IsMemberFunction, CallType, Loc, Range,
-                           CheckedVarArgs);
+                           CheckedVarArgs, isSprintf);
     }
   }
 
   // Refuse POD arguments that weren't caught by the format string
   // checks above.
   if (CallType != VariadicDoesNotApply) {
     unsigned NumParams = Proto ? Proto->getNumParams()
                        : FDecl && isa<FunctionDecl>(FDecl)
                            ? cast<FunctionDecl>(FDecl)->getNumParams()
                        : FDecl && isa<ObjCMethodDecl>(FDecl)
                            ? cast<ObjCMethodDecl>(FDecl)->param_size()
                        : 0;
 
     for (unsigned ArgIdx = NumParams; ArgIdx < Args.size(); ++ArgIdx) {
       // Args[ArgIdx] can be null in malformed code.
       if (const Expr *Arg = Args[ArgIdx]) {
         if (CheckedVarArgs.empty() || !CheckedVarArgs[ArgIdx])
           checkVariadicArgument(Arg, CallType);
       }
     }
   }
 
   if (FDecl || Proto) {
     CheckNonNullArguments(*this, FDecl, Proto, Args, Loc);
 
     // Type safety checking.
     if (FDecl) {
       for (const auto *I : FDecl->specific_attrs<ArgumentWithTypeTagAttr>())
         CheckArgumentWithTypeTag(I, Args.data());
     }
   }
 }
 
 /// CheckConstructorCall - Check a constructor call for correctness and safety
 /// properties not enforced by the C type system.
 void Sema::CheckConstructorCall(FunctionDecl *FDecl,
                                 ArrayRef<const Expr *> Args,
                                 const FunctionProtoType *Proto,
                                 SourceLocation Loc) {

 /// CheckFormatArguments - Check calls to printf and scanf (and similar
 /// functions) for correct use of format strings.
 /// Returns true if a format string has been fully checked.
 bool Sema::CheckFormatArguments(const FormatAttr *Format,
                                 ArrayRef<const Expr *> Args,
                                 bool IsCXXMember,
                                 VariadicCallType CallType,
                                 SourceLocation Loc, SourceRange Range,
-                                llvm::SmallBitVector &CheckedVarArgs) {
+                                llvm::SmallBitVector &CheckedVarArgs,
+                                bool isSprintf) {
   FormatStringInfo FSI;
   if (getFormatStringInfo(Format, IsCXXMember, &FSI))
     return CheckFormatArguments(Args, FSI.HasVAListArg, FSI.FormatIdx,
                                 FSI.FirstDataArg, GetFormatStringType(Format),
-                                CallType, Loc, Range, CheckedVarArgs);
+                                CallType, Loc, Range, CheckedVarArgs,
+                                isSprintf);
   return false;
 }
 
 bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
                                 bool HasVAListArg, unsigned format_idx,
                                 unsigned firstDataArg, FormatStringType Type,
                                 VariadicCallType CallType,
                                 SourceLocation Loc, SourceRange Range,
-                                llvm::SmallBitVector &CheckedVarArgs) {
+                                llvm::SmallBitVector &CheckedVarArgs,
+                                bool isSprintf) {
   // CHECK: printf/scanf-like function is called with no format string.
   if (format_idx >= Args.size()) {
     Diag(Loc, diag::warn_missing_format_string) << Range;
     return false;
   }
 
+  if (isSprintf)
+    Type = FST_Sprintf;
+
   const Expr *OrigFormatExpr = Args[format_idx]->IgnoreParenCasts();
 
   // CHECK: format string is not a string literal.
   //
   // Dynamically generated format strings are difficult to
   // automatically vet at compile time.  Requiring that format strings
   // are string literals: (1) permits the checking of format strings by
   // the compiler and thereby (2) can practically remove the source of

 
 // Note that this may return NULL if there was an error parsing or building
 // one of the argument expressions.
 const Expr *CheckFormatHandler::getDataArg(unsigned i) const {
   return Args[FirstDataArg + i];
 }
 
 void CheckFormatHandler::DoneProcessing() {
     // Does the number of data arguments exceed the number of
     // format conversions in the format string?
   if (!HasVAListArg) {
       // Find any arguments that weren't covered.
     CoveredArgs.flip();
     signed notCoveredArg = CoveredArgs.find_first();
     if (notCoveredArg >= 0) {
       assert((unsigned)notCoveredArg < NumDataArgs);
       if (const Expr *E = getDataArg((unsigned) notCoveredArg)) {
const Sema::SemaDiagnosticBuilder &Note =
       S.Diag(IsStringLocation ? Loc : StringRange.getBegin(),
              diag::note_format_string_defined);
 
     Note << StringRange;
     Note << FixIt;
   }
 }
 
-//===--- CHECK: Printf format string checking ------------------------------===//
+//===--- CHECK: Printf format string checking -----------------------------===//
 
 namespace {
 class CheckPrintfHandler : public CheckFormatHandler {
   bool ObjCContext;
 public:
   CheckPrintfHandler(Sema &s, const StringLiteral *fexpr,
                      const Expr *origFormatExpr, unsigned firstDataArg,
                      unsigned numDataArgs, bool isObjC,
                      const char *beg, bool hasVAListArg,
                      ArrayRef<const Expr *> Args,
                      unsigned formatIdx, bool inFunctionCall,
                      Sema::VariadicCallType CallType,
-                     llvm::SmallBitVector &CheckedVarArgs)
+                     llvm::SmallBitVector &CheckedVarArgs,
+                     bool isSprintf)
     : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg,
                          numDataArgs, beg, hasVAListArg, Args,
                          formatIdx, inFunctionCall, CallType, CheckedVarArgs),
-      ObjCContext(isObjC)
-  {}
+      ObjCContext(isObjC), isSprintf(isSprintf)
+  {
+    SprintfRequiredStorage = 1;
+  }
 
 
   bool HandleInvalidPrintfConversionSpecifier(
                                       const analyze_printf::PrintfSpecifier &FS,
                                       const char *startSpecifier,
                                       unsigned specifierLen) override;
 
   bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
                              const char *startSpecifier,
                              unsigned specifierLen) override;
   bool checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
                        const char *StartSpecifier,
                        unsigned SpecifierLen,
                        const Expr *E);
 
+  bool HandleSprintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                              const char *startSpecifier,
+                              unsigned specifierLen);
+
   bool HandleAmount(const analyze_format_string::OptionalAmount &Amt, unsigned k,
                     const char *startSpecifier, unsigned specifierLen);
   void HandleInvalidAmount(const analyze_printf::PrintfSpecifier &FS,
                            const analyze_printf::OptionalAmount &Amt,
                            unsigned type,
                            const char *startSpecifier, unsigned specifierLen);
   void HandleFlag(const analyze_printf::PrintfSpecifier &FS,
                   const analyze_printf::OptionalFlag &flag,
                   const char *startSpecifier, unsigned specifierLen);
   void HandleIgnoredFlag(const analyze_printf::PrintfSpecifier &FS,
                          const analyze_printf::OptionalFlag &ignoredFlag,
                          const analyze_printf::OptionalFlag &flag,
                          const char *startSpecifier, unsigned specifierLen);
   bool checkForCStrMembers(const analyze_printf::ArgType &AT,
                            const Expr *E);
-
-};  
+private:
+  bool isSprintf;
+  uint64_t SprintfRequiredStorage;
+};
 }
 
 bool CheckPrintfHandler::HandleInvalidPrintfConversionSpecifier(
                                       const analyze_printf::PrintfSpecifier &FS,
                                       const char *startSpecifier,
                                       unsigned specifierLen) {
   const analyze_printf::PrintfConversionSpecifier &CS =
     FS.getConversionSpecifier();
CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier
 
   if (!CheckNumArgs(FS, CS, startSpecifier, specifierLen, argIndex))
     return false;
 
   const Expr *Arg = getDataArg(argIndex);
   if (!Arg)
     return true;
 
-  return checkFormatExpr(FS, startSpecifier, specifierLen, Arg);
+
+  if (checkFormatExpr(FS, startSpecifier, specifierLen, Arg)) {
+    if (isSprintf)
+      return HandleSprintfSpecifier(FS, startSpecifier, specifierLen);
+    else
+      return true;
+  }
+  return false;
+}
+
+static const ConstantArrayType *getConstArrayType(Sema &S, const Expr *Array) {
+  return S.Context.getAsConstantArrayType(Array->getType());
+}
+
+// Analyses the data arguments to sprintf to warn the user of memory overflows.
+// Mainly handles integer and string literals with their specifiers and flags
+// with little support for floating point values. None specifier text is also
+// not considered towards the storage space required for the string.
+bool CheckPrintfHandler::HandleSprintfSpecifier(
+  const analyze_printf::PrintfSpecifier &FS,
+  const char *startSpecifier, unsigned specifierLen) {
+
+  using namespace analyze_format_string;
+  using namespace analyze_printf;
+
+  const PrintfConversionSpecifier &CS = FS.getConversionSpecifier();
+  ConversionSpecifier::Kind CSKind = CS.getKind();
+  OptionalAmount Precision = FS.getPrecision();
+  OptionalAmount Width = FS.getFieldWidth();
+  // Offset the argument index to the data items to be printed.
+  unsigned Idx = 4 + FS.getArgIndex();
+  const Expr* Data = Args[Idx]->IgnoreImplicit();
+  uint64_t StorageBufSize = 0;
+  uint64_t RequiredStorage = 0;
+
+  if (auto *StorageBufTy = getConstArrayType(S, Args[0]->IgnoreImplicit()))
+    StorageBufSize = StorageBufTy->getSize().getZExtValue();
+  else
+    return true;
+
+  switch (CSKind) {
+  default:
+    // We can't handle eArg, EArg, fArg, FArg, gArg, GArg, nArg
+    break;
+  case ConversionSpecifier::aArg:
+  case ConversionSpecifier::AArg: {
+    if (isa<FloatingLiteral>(Data)) {
+      llvm::APFloat Value = cast<FloatingLiteral>(Data)->getValue();
+      char TempBuf[32];
+      unsigned HexDigits = 0;
+
+      if (Precision.getHowSpecified() == OptionalAmount::Constant)
+        HexDigits = Precision.getConstantAmount();
+
+      // Get the number of digits used to print the value.
+      unsigned NumDigits =
+        Value.convertToHexString(TempBuf, HexDigits, false,
+                                 llvm::APFloat::rmNearestTiesToEven);
+      RequiredStorage += NumDigits;
+    }
+    break;
+  }
+  case ConversionSpecifier::cArg:
+  case ConversionSpecifier::CArg:
+   ++RequiredStorage;
+   break;
+  case ConversionSpecifier::dArg:
+  case ConversionSpecifier::DArg:
+  case ConversionSpecifier::iArg:
+  case ConversionSpecifier::uArg:
+  case ConversionSpecifier::UArg: {
+    if (Data->isEvaluatable(S.getASTContext())) {
+      llvm::APSInt Result = llvm::APSInt(64);
+      if (CSKind == ConversionSpecifier::uArg ||
+          CSKind == ConversionSpecifier::UArg)
+        Result = llvm::APSInt(64, false);
+      if (Data->EvaluateAsInt(Result, S.getASTContext())) {
+        std::string DataStr = Result.toString(10, true);
+        RequiredStorage += DataStr.length();
+      }
+    }
+    break;
+  }
+  case ConversionSpecifier::pArg: {
+    // When using the %p specifier, all platforms appear to use hex digits, this
+    // is prepended with '0x' on BSD, Linux and OSX.
+    const TargetInfo &TI = S.Context.getTargetInfo();
+    RequiredStorage += TI.getPointerWidth(0) / 4;
+    if (TI.getTriple().isOSDarwin() ||
+        TI.getTriple().isOSLinux() ||
+        TI.getTriple().isOSNetBSD() ||
+        TI.getTriple().isOSOpenBSD() ||
+        TI.getTriple().isOSFreeBSD() ||
+        TI.getTriple().isOSDragonFly())
+      RequiredStorage += 2;
+    break;
+  }
+  case ConversionSpecifier::sArg:
+  case ConversionSpecifier::SArg:
+  case ConversionSpecifier::ZArg:
+    if (auto SL = dyn_cast<StringLiteral>(Data))
+      RequiredStorage += SL->getByteLength();
+    else if (auto DataBuf = getConstArrayType(S, Data)) {
+      uint64_t BufSize = DataBuf->getSize().getZExtValue();
+      RequiredStorage += BufSize;
+    }
+    break;
+  case ConversionSpecifier::oArg:
+  case ConversionSpecifier::OArg:
+  case ConversionSpecifier::xArg:
+  case ConversionSpecifier::XArg: {
+    unsigned Radix = 16;
+    if (CSKind == ConversionSpecifier::oArg ||
+        CSKind == ConversionSpecifier::OArg)
+      Radix = 8;
+    if (isa<IntegerLiteral>(Data)) {
+      // Create a real string to use its size.
+      std::string DataStr =
+        cast<IntegerLiteral>(Data)->getValue().toString(Radix, true);
+      RequiredStorage += DataStr.length();
+      // 0x will can be prepended for oct and hex values.
+      if (FS.hasAlternativeForm())
+        RequiredStorage += 2;
+    }
+    break;
+  }
+  }
+
+  // An extra will be required for +/-
+  if (FS.hasPlusPrefix() || FS.hasSpacePrefix())
+    ++RequiredStorage;
+
+  // The calculated sizes could be smaller than sizes specified by the user
+  // in their format string.
+  if (CS.isAnyIntArg()) {
+    // For integers, the precision modifier specifies how many digits to be
+    // written so this may overwrite the calculated size.
+    if (Precision.getHowSpecified() == OptionalAmount::Constant) {
+      if (RequiredStorage <= Precision.getConstantAmount())
+        RequiredStorage = Precision.getConstantAmount();
+    }
+  }
+
+  // Width specifies the minimum number of characters to be printed.
+  if (Width.getHowSpecified() == OptionalAmount::Constant) {
+    if (RequiredStorage <= Width.getConstantAmount())
+      RequiredStorage = Width.getConstantAmount();
+  }
+
+  // Update the total required.
+  SprintfRequiredStorage += RequiredStorage;
+
+  if (StorageBufSize < SprintfRequiredStorage) {
+    std::string Limit = llvm::APInt(64, StorageBufSize).toString(10, false);
+    std::string Required =
+      llvm::APInt(64, SprintfRequiredStorage).toString(10, false);
+    S.Diag(Args[0]->getLocStart(), diag::warn_sprintf_chk_overflow)
+      << getSpecifierRange(startSpecifier, specifierLen)
+      << Data->getSourceRange()
+      << Required
+      << Limit;
+    return false;
+  }
+
+  return true;
 }
 
 static bool requiresParensToAddCast(const Expr *E) {
   // FIXME: We should have a general way to reason about operator
   // precedence and whether parens are actually needed here.
   // Take care of a few common cases where they aren't.
   const Expr *Inside = E->IgnoreImpCasts();
   if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(Inside))
     Inside = POE->getSyntacticForm()->IgnoreImpCasts();
 
   switch (Inside->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass:
   case Stmt::CallExprClass:
   case Stmt::CharacterLiteralClass:
   case Stmt::CXXBoolLiteralExprClass:
   case Stmt::DeclRefExprClass:
   case Stmt::FloatingLiteralClass:
   case Stmt::IntegerLiteralClass:
   case Stmt::MemberExprClass:
   case Stmt::ObjCArrayLiteralClass:
   case Stmt::ObjCBoolLiteralExprClass:
   case Stmt::ObjCBoxedExprClass:
   case Stmt::ObjCDictionaryLiteralClass:
   case Stmt::ObjCEncodeExprClass:
   case Stmt::ObjCIvarRefExprClass:
   case Stmt::ObjCMessageExprClass:
   case Stmt::ObjCPropertyRefExprClass:
   case Stmt::ObjCStringLiteralClass:
   case Stmt::ObjCSubscriptRefExprClass:
   case Stmt::ParenExprClass:
   case Stmt::StringLiteralClass:
   case Stmt::UnaryOperatorClass:
     return false;
   default:
     return true;
   }
 }
 
 static std::pair<QualType, StringRef>
 shouldNotPrintDirectly(const ASTContext &Context,
                        QualType IntendedTy,
                        const Expr *E) {
   // Use a 'while' to peel off layers of typedefs.
shouldNotPrintDirectly(const ASTContext &Context,
   }
 
   // Strip parens if necessary.
   if (const ParenExpr *PE = dyn_cast<ParenExpr>(E))
     return shouldNotPrintDirectly(Context,
                                   PE->getSubExpr()->getType(),
                                   PE->getSubExpr());
 
   // If this is a conditional expression, then its result type is constructed
   // via usual arithmetic conversions and thus there might be no necessary
   // typedef sugar there.  Recurse to operands to check for NSInteger &
   // Co. usage condition.
   if (const ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {
     QualType TrueTy, FalseTy;
     StringRef TrueName, FalseName;
 
     std::tie(TrueTy, TrueName) =
       shouldNotPrintDirectly(Context,
                              CO->getTrueExpr()->getType(),
                              CO->getTrueExpr());
     std::tie(FalseTy, FalseName) =
if (match == analyze_printf::ArgType::Match) {
     return true;
   }
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
   // and function pointer decay; seeing that an argument intended to be a
   // string has type 'char [6]' is probably more confusing than 'char *'.
   if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
     if (ICE->getCastKind() == CK_IntegralCast ||
         ICE->getCastKind() == CK_FloatingCast) {
       E = ICE->getSubExpr();
       ExprTy = E->getType();
 
       // Check if we didn't match because of an implicit cast from a 'char'
       // or 'short' to an 'int'.  This is done because printf is a varargs
       // function.
       if (ICE->getType() == S.Context.IntTy ||
           ICE->getType() == S.Context.UnsignedIntTy) {
         // All further checking is done on the subexpression.
         if (AT.matchesType(S.Context, ExprTy))
           return true;
       }
     }
   } else if (const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)) {
     // Special case for 'a', which has type 'int' in C.
     // Note, however, that we do /not/ want to treat multibyte constants like
     // 'MooV' as characters! This form is deprecated but still exists.
     if (ExprTy == S.Context.IntTy)
       if (llvm::isUIntN(S.Context.getCharWidth(), CL->getValue()))
         ExprTy = S.Context.CharTy;
   }
 
   // Look through enums to their underlying type.
   bool IsEnum = false;
   if (auto EnumTy = ExprTy->getAs<EnumType>()) {
     ExprTy = EnumTy->getDecl()->getIntegerType();
     IsEnum = true;
   }
 
   // %C in an Objective-C context prints a unichar, not a wchar_t.
   // If the argument is an integer of some kind, believe the %C and suggest
   // a cast instead of changing the conversion specifier.

 }
 
 bool CheckScanfHandler::HandleScanfSpecifier(
                                        const analyze_scanf::ScanfSpecifier &FS,
                                        const char *startSpecifier,
                                        unsigned specifierLen) {
   
   using namespace analyze_scanf;
   using namespace analyze_format_string;  
 
   const ScanfConversionSpecifier &CS = FS.getConversionSpecifier();
 
   // Handle case where '%' and '*' don't consume an argument.  These shouldn't
   // be used to decide if we are using positional arguments consistently.
   if (FS.consumesDataArgument()) {
     if (atFirstArg) {
       atFirstArg = false;
       usesPositionalArgs = FS.usesPositionalArg();
     }
     else if (usesPositionalArgs != FS.usesPositionalArg()) {
       HandlePositionalNonpositionalArgs(getLocationOfByte(CS.getStart()),
                                         startSpecifier, specifierLen);
       return false;
     }
if (StrLen == 0 && numDataArgs > 0) {
     CheckFormatHandler::EmitFormatDiagnostic(
       *this, inFunctionCall, Args[format_idx],
       PDiag(diag::warn_empty_format_string), FExpr->getLocStart(),
       /*IsStringLocation*/true, OrigFormatExpr->getSourceRange());
     return;
   }
   
   if (Type == FST_Printf || Type == FST_NSString ||
-      Type == FST_FreeBSDKPrintf || Type == FST_OSTrace) {
+      Type == FST_FreeBSDKPrintf || Type == FST_OSTrace ||
+      Type == FST_Sprintf) {
     CheckPrintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg,
-                         numDataArgs, (Type == FST_NSString || Type == FST_OSTrace),
+                         numDataArgs,
+                         (Type == FST_NSString || Type == FST_OSTrace),
                          Str, HasVAListArg, Args, format_idx,
-                         inFunctionCall, CallType, CheckedVarArgs);
+                         inFunctionCall, CallType, CheckedVarArgs,
+                         Type == FST_Sprintf);
   
     if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen,
                                                   getLangOpts(),
                                                   Context.getTargetInfo(),
                                                   Type == FST_FreeBSDKPrintf))
       H.DoneProcessing();
   } else if (Type == FST_Scanf) {
     CheckScanfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, numDataArgs,

test/Sema/sprintf-chk.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fsyntax-only %s
+void test() {
+
+  char bufA[4];
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%s", "foo");
+  char addrBuf[19];
+  __builtin___sprintf_chk(addrBuf, 0, __builtin_object_size(addrBuf, 0), "%p",
+                          bufA);
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d%d", 10, 10);
+  // expected-warning@-1{{call will always overflow destination buffer as it requires at least 5 bytes whereas 4 are provided}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i%i", 10, 10);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u%u", 10, 10);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x%x", 100, 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X%X", 100, 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o%o", 100, 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d", 1000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i", 1000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u", 1000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x", 10000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X", 10000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o", 1000);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%a", 10.0);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%A", 10.0);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%s", "foobar");
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%p", bufA);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%+d", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%+d", 10);
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3d", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3i", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3u", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3x", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3X", 100);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3o", 100);
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4d", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4i", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4u", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4x", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4X", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4o", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#x", 15);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#X", 15);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#o", 7);
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#x", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#X", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#o", 100);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+
+
+  char bufB[7];
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%a", 0.5);
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%A", 0.5);
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%a", 10.0);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%A", 10.0);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.1a", 0.5);
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.1A", 0.5);
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.2a", 0.5);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.2A", 0.5);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+
+  char bufC[5];
+  __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s%s",
+                          "foo", "bar");
+  // expected-warning@-2{{call will always overflow destination buffer}}
+  const char bar[] = "foobar";
+  __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s", bar);
+  // expected-warning@-1{{call will always overflow destination buffer}}
+}