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

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

13

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/Sema/Sema.h

   ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,
                           unsigned BuiltinID, CallExpr *TheCall);
 
+  void CheckSprintfMemAlloc(FunctionDecl *FDecl, CallExpr *TheCall,
+                             unsigned BufIdx, unsigned FmtIdx,
+                             unsigned DataIdx);
+
   bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
                                     unsigned MaxWidth);
   bool CheckNeonBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall);

   enum FormatStringType {
     FST_Scanf,
     FST_Printf,
+    FST_Sprintf,
     FST_NSString,
     FST_Strftime,
     FST_Strfmon,

lib/Sema/SemaChecking.cpp

   case Builtin::BI__builtin___stpncpy_chk:
     SemaBuiltinMemChkCall(*this, FDecl, TheCall, 2, 3);
     break;
+  case Builtin::BI__builtin___sprintf_chk:
+    CheckSprintfMemAlloc(FDecl, TheCall, 0, 3, 4);
+    break;
   case Builtin::BI__builtin___memccpy_chk:
     SemaBuiltinMemChkCall(*this, FDecl, TheCall, 3, 4);
     break;

   return false;
 }
 
+void Sema::CheckSprintfMemAlloc(FunctionDecl *FDecl, CallExpr *TheCall,
+                                unsigned BufIdx, unsigned FmtIdx,
+                                unsigned DataIdx) {
+  unsigned NumArgs = TheCall->getNumArgs();
+  if (NumArgs <= BufIdx ||
+      NumArgs <= FmtIdx ||
+      NumArgs <= DataIdx)
+    return;
+
+  const Expr *FmtArg = TheCall->getArg(FmtIdx);
+  if (!isa<StringLiteral>(FmtArg->IgnoreImplicit()))
+    return;
+
+  const FunctionProtoType *Proto =
+    FDecl->getType()->castAs<FunctionProtoType>();
+  VariadicCallType CallType = getVariadicCallType(FDecl, Proto,
+                                                    TheCall->getCallee());
+  llvm::ArrayRef<const Expr*> Args =
+    llvm::makeArrayRef(TheCall->getArgs(), NumArgs);
+  llvm::SmallBitVector CheckedVarArgs;
+  if (FDecl) {
+    for (const auto *I : FDecl->specific_attrs<FormatAttr>()) {
+      // Only create vector if there are format attributes.
+      CheckedVarArgs.resize(Args.size());
+      CheckFormatArguments(I, Args, /*IsMemberFunction=*/false, CallType,
+                           TheCall->getLocStart(), FDecl->getSourceRange(),
+                           CheckedVarArgs);
+    }
+  }
+  const StringLiteral *SL = cast<StringLiteral>(FmtArg->IgnoreImplicit());
+  CheckFormatString(SL, FmtArg, Args, true, FmtIdx,  DataIdx, FST_Sprintf,
+                    /*inFunctionCall=*/true, CallType, CheckedVarArgs);
+}
+
 bool Sema::CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation lbrac, 
                                ArrayRef<const Expr *> Args) {
   VariadicCallType CallType =

   bool checkForCStrMembers(const analyze_printf::ArgType &AT,
                            const Expr *E);
 
-};  
+};
 }
 
 bool CheckPrintfHandler::HandleInvalidPrintfConversionSpecifier(

   return true;
 }
 
+static const ConstantArrayType* GetArrayType(Sema &S, const Expr *Array) {
+  if (isa<DeclRefExpr>(Array)) {
+    QualType ArrayTy = cast<DeclRefExpr>(Array)->getDecl()->getType();
+    if (ArrayTy->isConstantArrayType())
+      return S.Context.getAsConstantArrayType(Array->getType());
+  }
+  return nullptr;
+}
+
+// CHECK: Sprintf format string checking, uses printf checking with added data
+// size analysing to warn of memory overflows.
+namespace {
+  class CheckSprintfHandler : public CheckPrintfHandler {
+  public:
+    CheckSprintfHandler(Sema &S, const StringLiteral *FExpr,
+                        const Expr *OrigFormatExpr, unsigned FirstDataArg,
+                        unsigned numDataArgs, bool isObjC,
+                        const char *Begin, bool hasVAListArg,
+                        ArrayRef<const Expr*> Args,
+                        unsigned FormatIdx, bool inFunctionCall,
+                        Sema::VariadicCallType CallType,
+                        llvm::SmallBitVector &CheckedVarArgs)
+      : CheckPrintfHandler(S, FExpr, OrigFormatExpr, FirstDataArg, numDataArgs,
+                           isObjC, Begin, hasVAListArg, Args, FormatIdx,
+                           inFunctionCall, CallType, CheckedVarArgs),
+      Args(Args), S(S) {
+        StorageBufSize = llvm::APInt(64, 0);
+        StorageRequired = llvm::APInt(64, 0);
+      }
+
+    bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                               const char *startSpecifier,
+                               unsigned specifierLen) override;
+
+    bool Init() {
+      if (const ConstantArrayType *StorageBufTy =
+        GetArrayType(S, Args[0]->IgnoreImplicit())) {
+        StorageBufSize = StorageBufTy->getSize();
+        return true;
+      }
+      return false;
+    }
+
+  private:
+    ArrayRef<const Expr*> Args;
+    Sema &S;
+    llvm::APInt StorageBufSize;
+    llvm::APInt StorageRequired;
+  };
+}
+
+// Analyses the data arguments to sprintf to warn the user of memory overflows.
+// Mainly handles integer and string literals with their specifiers and flags.
+bool
+CheckSprintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier
+                                           &FS, const char *startSpecifier,
+                                           unsigned specifierLen) {
+  // Call the parent call first to do perform the validation on the specifier.
+  if (!CheckPrintfHandler::HandlePrintfSpecifier(FS, startSpecifier,
+                                                 specifierLen))
+    return false;
+
+  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();
+
+  // An extra will be required for +/-
+  if (FS.hasPlusPrefix() || FS.hasSpacePrefix())
+    ++StorageRequired;
+
+  if (isa<StringLiteral>(Data)) {
+    auto SL = cast<StringLiteral>(Data);
+    StorageRequired += llvm::APInt(64, SL->getByteLength());
+  }
+  else if (isa<IntegerLiteral>(Data)) {
+    // Default to decimal representation.
+    unsigned Radix = 10;
+
+    // Unsigned hexadecimal integers and pointer addresses.
+    if (CSKind == ConversionSpecifier::xArg ||
+        CSKind == ConversionSpecifier::XArg ||
+        CSKind == ConversionSpecifier::pArg)
+      Radix = 16;
+
+    // Unsigned octal.
+    else if (CSKind == ConversionSpecifier::oArg ||
+             CSKind == ConversionSpecifier::OArg)
+      Radix = 8;
+
+    // Create a real string to use its size.
+    std::string DataStr =
+      cast<IntegerLiteral>(Data)->getValue().toString(Radix, true);
+    StorageRequired += llvm::APInt(64, DataStr.length());
+  }
+  else if (isa<FloatingLiteral>(Data)) {
+    llvm::APFloat Value = cast<FloatingLiteral>(Data)->getValue();
+
+    // APFloat has a member function for printing the value to a hex
+    // representation.
+    if (CSKind == ConversionSpecifier::aArg ||
+        CSKind == ConversionSpecifier::AArg) {
+      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);
+      StorageRequired += llvm::APInt(64, NumDigits);
+    }
+  }
+  else if (auto DataBuf = GetArrayType(S, Data)) {
+    llvm::APInt BufSize = DataBuf->getSize();
+    StorageRequired += BufSize;
+  }
+
+  if (FS.hasAlternativeForm()) {
+    // 0x will be prepended for oct and hex values.
+    if (CSKind == ConversionSpecifier::xArg ||
+        CSKind == ConversionSpecifier::XArg ||
+        CSKind == ConversionSpecifier::oArg ||
+        CSKind == ConversionSpecifier::OArg)
+      StorageRequired += llvm::APInt(64, 2);
+  }
+
+  // Pointer address have 'p' prepended
+  if (CSKind == ConversionSpecifier::pArg)
+    ++StorageRequired;
+
+  // 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 (StorageRequired.ule(Precision.getConstantAmount()))
+        StorageRequired = llvm::APInt(64, Precision.getConstantAmount());
+    }
+  }
+
+  // Width specifies the minimum number of characters to be printed.
+  if (Width.getHowSpecified() == OptionalAmount::Constant) {
+    if (StorageRequired.ule(Width.getConstantAmount()))
+      StorageRequired = llvm::APInt(64, Width.getConstantAmount());
+  }
+
+  // Compare less or equal to account for the null terminator that will
+  // be appended.
+  if (StorageBufSize.ule(StorageRequired)) {
+    //S.Diag(Args[0]->getLocStart(), diag::warn_sprintf_chk_overflow)
+    S.Diag(Args[0]->getLocStart(), diag::warn_memcpy_chk_overflow)
+      << getSpecifierRange(startSpecifier, specifierLen)
+      << Data->getSourceRange()
+      << Args[Idx]->getType();
+    return false;
+  }
+
+  return true;
+}
+
 //===--- CHECK: Scanf format string checking ------------------------------===//
 
 namespace {  

                                                  getLangOpts(),
                                                  Context.getTargetInfo()))
       H.DoneProcessing();
+  }
+  else if (Type == FST_Sprintf) {
+    // Sprintf has the same parsing and checking as printf, but we can also
+    // perform memory overflow checks.
+    CheckSprintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg,
+                          numDataArgs,
+                          (Type == FST_NSString || Type == FST_OSTrace),
+                          Str, HasVAListArg, Args, format_idx,
+                          inFunctionCall, CallType, CheckedVarArgs);
+    if (H.Init()) {
+      if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen,
+                                                    getLangOpts(),
+                                                    Context.getTargetInfo(),
+                                                    Type == FST_FreeBSDKPrintf))
+        H.DoneProcessing();
+    }
   } // TODO: handle other formats
 }
 

test/SemaCXX/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");
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d%d", 10, 10);   // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i%i", 10, 10);   // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u%u", 10, 10);   // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x%x", 100, 100); // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X%X", 100, 100); // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o%o", 100, 100); // expected-warning {{will always overflow destination buffer}}
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d", 1000);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i", 1000);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u", 1000);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x", 10000);    // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X", 10000);    // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o", 1000);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%a", 10.0);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%A", 10.0);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%s", "foobar"); // expected-warning {{will always overflow destination buffer}}
+
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%+d", 100);     // expected-warning {{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 {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4i", 100);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4u", 100);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4x", 100);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4X", 100);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4o", 100);     // expected-warning {{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 {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#X", 100);     // expected-warning {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#o", 100);     // expected-warning {{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 {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%A", 10.0);     // expected-warning {{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 {{will always overflow destination buffer}}
+  __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.2A", 0.5);   // expected-warning {{will always overflow destination buffer}}
+
+  char bufC[5];
+  __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s%s",          // expected-warning {{will always overflow destination buffer}}
+                          "foo", "bar");
+  const char bar[] = "foobar";
+  __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s", bar);      // expected-warning {{will always overflow destination buffer}}
+}