[analyzer] Report CFNumberGetValue API misuse

This patch contains 2 improvements to the CFNumber checker:
 - Checking of CFNumberGetValue misuse.
 - Treating all CFNumber API misuse errors as non-fatal. (Previously we treated errors that could cause uninitialized memory as syncs and the truncation errors as non-fatal.)

This implements a subset of functionality from https://reviews.llvm.org/D17954.

Differential Revision: https://reviews.llvm.org/D25876

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@285253 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td
index f1f0d4bade39694da3d72024ac07bca107227bb5..16ccea615af000e26fbd98118567f6573dc4c0de 100644 (file)
--- a/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -586,8 +586,8 @@ def DirectIvarAssignmentForAnnotatedFunctions : Checker<"DirectIvarAssignmentFor
 
 let ParentPackage = CoreFoundation in {
 
-def CFNumberCreateChecker : Checker<"CFNumber">,
-  HelpText<"Check for proper uses of CFNumberCreate">,
+def CFNumberChecker : Checker<"CFNumber">,
+  HelpText<"Check for proper uses of CFNumber APIs">,
   DescFile<"BasicObjCFoundationChecks.cpp">;
 
 def CFRetainReleaseChecker : Checker<"CFRetainRelease">,

diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
index 6239c5507a4be2239b302a03d19463901e36ecbd..1ea85d60c9e923316d7aed9239296484c4ddabe9 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@ void NilArgChecker::checkPostStmt(const ObjCDictionaryLiteral *DL,
 }
 
 //===----------------------------------------------------------------------===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
 //===----------------------------------------------------------------------===//
 
 namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt<CallExpr> > {
+class CFNumberChecker : public Checker< check::PreStmt<CallExpr> > {
   mutable std::unique_ptr<APIMisuse> BT;
-  mutable IdentifierInfo* II;
+  mutable IdentifierInfo *ICreate, *IGetValue;
 public:
-  CFNumberCreateChecker() : II(nullptr) {}
+  CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
 
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
 
@@ -425,7 +425,7 @@ static const char* GetCFNumberTypeStr(uint64_t i) {
 }
 #endif
 
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
                                          CheckerContext &C) const {
   ProgramStateRef state = C.getState();
   const FunctionDecl *FD = C.getCalleeDecl(CE);
@@ -433,10 +433,12 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
     return;
 
   ASTContext &Ctx = C.getASTContext();
-  if (!II)
-    II = &Ctx.Idents.get("CFNumberCreate");
-
-  if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+  if (!ICreate) {
+    ICreate = &Ctx.Idents.get("CFNumberCreate");
+    IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+  }
+  if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+      CE->getNumArgs() != 3)
     return;
 
   // Get the value of the "theType" argument.
@@ -450,13 +452,13 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
     return;
 
   uint64_t NumberKind = V->getValue().getLimitedValue();
-  Optional<uint64_t> OptTargetSize = GetCFNumberSize(Ctx, NumberKind);
+  Optional<uint64_t> OptCFNumberSize = GetCFNumberSize(Ctx, NumberKind);
 
   // FIXME: In some cases we can emit an error.
-  if (!OptTargetSize)
+  if (!OptCFNumberSize)
     return;
 
-  uint64_t TargetSize = *OptTargetSize;
+  uint64_t CFNumberSize = *OptCFNumberSize;
 
   // Look at the value of the integer being passed by reference.  Essentially
   // we want to catch cases where the value passed in is not equal to the
@@ -481,39 +483,44 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
   if (!T->isIntegralOrEnumerationType())
     return;
 
-  uint64_t SourceSize = Ctx.getTypeSize(T);
+  uint64_t PrimitiveTypeSize = Ctx.getTypeSize(T);
 
-  // CHECK: is SourceSize == TargetSize
-  if (SourceSize == TargetSize)
+  if (PrimitiveTypeSize == CFNumberSize)
     return;
 
-  // Generate an error.  Only generate a sink error node
-  // if 'SourceSize < TargetSize'; otherwise generate a non-fatal error node.
-  //
   // FIXME: We can actually create an abstract "CFNumber" object that has
   //  the bits initialized to the provided values.
-  //
-  ExplodedNode *N = SourceSize < TargetSize ? C.generateErrorNode()
-                                            : C.generateNonFatalErrorNode();
+  ExplodedNode *N = C.generateNonFatalErrorNode();
   if (N) {
     SmallString<128> sbuf;
     llvm::raw_svector_ostream os(sbuf);
+    bool isCreate = (FD->getIdentifier() == ICreate);
+
+    if (isCreate) {
+      os << (PrimitiveTypeSize == 8 ? "An " : "A ")
+         << PrimitiveTypeSize << "-bit integer is used to initialize a "
+         << "CFNumber object that represents "
+         << (CFNumberSize == 8 ? "an " : "a ")
+         << CFNumberSize << "-bit integer; ";
+    } else {
+      os << "A CFNumber object that represents "
+         << (CFNumberSize == 8 ? "an " : "a ")
+         << CFNumberSize << "-bit integer is used to initialize "
+         << (PrimitiveTypeSize == 8 ? "an " : "a ")
+         << PrimitiveTypeSize << "-bit integer; ";
+    }
 
-    os << (SourceSize == 8 ? "An " : "A ")
-       << SourceSize << " bit integer is used to initialize a CFNumber "
-                        "object that represents "
-       << (TargetSize == 8 ? "an " : "a ")
-       << TargetSize << " bit integer. ";
-
-    if (SourceSize < TargetSize)
-      os << (TargetSize - SourceSize)
-      << " bits of the CFNumber value will be garbage." ;
+    if (PrimitiveTypeSize < CFNumberSize)
+      os << (CFNumberSize - PrimitiveTypeSize)
+      << " bits of the CFNumber value will "
+      << (isCreate ? "be garbage." : "overwrite adjacent storage.");
     else
-      os << (SourceSize - TargetSize)
-      << " bits of the input integer will be lost.";
+      os << (PrimitiveTypeSize - CFNumberSize)
+      << " bits of the integer value will be "
+      << (isCreate ? "lost." : "garbage.");
 
     if (!BT)
-      BT.reset(new APIMisuse(this, "Bad use of CFNumberCreate"));
+      BT.reset(new APIMisuse(this, "Bad use of CFNumber APIs"));
 
     auto report = llvm::make_unique<BugReport>(*BT, os.str(), N);
     report->addRange(CE->getArg(2)->getSourceRange());
@@ -1272,8 +1279,8 @@ void ento::registerNilArgChecker(CheckerManager &mgr) {
   mgr.registerChecker<NilArgChecker>();
 }
 
-void ento::registerCFNumberCreateChecker(CheckerManager &mgr) {
-  mgr.registerChecker<CFNumberCreateChecker>();
+void ento::registerCFNumberChecker(CheckerManager &mgr) {
+  mgr.registerChecker<CFNumberChecker>();
 }
 
 void ento::registerCFRetainReleaseChecker(CheckerManager &mgr) {

diff --git a/test/Analysis/CFNumber.c b/test/Analysis/CFNumber.c
index ebb3b1ac1ed81abdbc05f7909f1ab5ae72455c18..ec966ee4d6c0f4b2038976e745527261c535c5cc 100644 (file)
--- a/test/Analysis/CFNumber.c
+++ b/test/Analysis/CFNumber.c
@@ -13,14 +13,16 @@ enum { kCFNumberSInt8Type = 1, kCFNumberSInt16Type = 2,
        kCFNumberMaxType = 16 };
 typedef CFIndex CFNumberType;
 typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
 extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
 
-CFNumberRef f1(unsigned char x) {
-  return CFNumberCreate(0, kCFNumberSInt16Type, &x);  // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
+  return CFNumberCreate(0, kCFNumberSInt16Type, &x);  // expected-warning{{An 8-bit integer is used to initialize a CFNumber object that represents a 16-bit integer; 8 bits of the CFNumber value will be garbage}}
 }
 
 __attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
-  return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+  return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16-bit integer is used to initialize a CFNumber object that represents an 8-bit integer; 8 bits of the integer value will be lost}}
 }
 
 // test that the attribute overrides the naming convention.
@@ -28,6 +30,18 @@ __attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x)
   return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
 }
 
-CFNumberRef f3(unsigned i) {
-  return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
+  return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32-bit integer is used to initialize a CFNumber object that represents a 64-bit integer}}
+}
+
+unsigned char getValueTest1(CFNumberRef x) {
+  unsigned char scalar = 0;
+  CFNumberGetValue(x, kCFNumberSInt16Type, &scalar);  // expected-warning{{A CFNumber object that represents a 16-bit integer is used to initialize an 8-bit integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+  return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+  unsigned short scalar = 0;
+  CFNumberGetValue(x, kCFNumberSInt8Type, &scalar);  // expected-warning{{A CFNumber object that represents an 8-bit integer is used to initialize a 16-bit integer; 8 bits of the integer value will be garbage}}
+  return scalar;
 }