Improvements to CFNumberCreate checker

[rgov]

Summary

This is an attempt at improving the CFNumber checker which catches disagreements between the programmer-declared CFNumberType of the value (e.g., kCFNumberSInt16Type) and its actual type (hopefully a signed short).

The existing code only checked the expected number of bits. However, there are other ways this could go wrong, such as passing a double where a 64-bit integer is expected, or accidentally interpreting an unsigned integer as a signed integer.

I consider the set of CFNumberTypes as two categories: regular numeric types that map cleanly to standard C data types, such as SInt8, Float, LongLong, etc., and special named types such as CFIndex, NSInteger, and Float32 (which is not necessarily a IEEE 754 float).

For numeric types, they need only agree in signedness, bit width, and numeric type (integer vs real).

For the special named types, I don't want to allow passing, for instance, an int when kCFNumberCFIndexType is specified—that might work without issue on 32-bit, but on 64-bit there will be an overflow. So for these named types, I want to make sure that you are actually passing a CFIndex value (or something typedef'd to CFIndex).

I also extended it to also check CFNumberGetValue, which has the same issues as CFNumberCreate.

The checker is incomplete (for instance, I'd welcome feedback on how the diagnostics should be written) but I'm posting here for some early feedback.

[rgov]

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

352

Since it handles floating point values as well, this function is misnamed.

502

I don't know how to print out the description of the type. Even though T is declared as a QualType, the compiler complains if I try to use T->getAsString() on it.

test/Analysis/CFNumber.c

85

This test is broken, I think because we discard casts, but I think it should be allowed—but we would still want to check that it would not overflow.

[zaks.anna]

It would be better to stage this as a separate checker and later delete the old checker. (Not a must fix though.) Why are the old tests deleted? My understanding is that the new checker should be more strict than the old one. Is it correct?

We'd need to run this on a bunch of code and see if it reports false positives. People might use unexpected coding patterns.

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

420

Better to use the new functionality committed in http://reviews.llvm.org/D15921.

441

Best to add a test case in addition to the FIXME. Would casts serve as a false positive suppression mechanism for users?

449

How about rewriting with amore readable version:

if (!checkIntegralType(NumberKind, T, CE, isCreate, C))
  return;
checkSpecialType(NumberKind, T, CE, isCreate, C);

502

I don't think you should mention the name of the expected type since there can be many acceptable ones. I'll get back to you on printing the type, but maybe describing what the issue is would be sufficient? Not sure.

509

We should change it to read as a sentence, even if the sentence is different in each case. For example, printing the target and source type sizes as before gives very valuable information. Type names might not be as useful when those are defined using complex preprocessor logic.

521

We should check if this utility already exists in the compiler somewhere.

566

This is copy and paste from checkIntegralType.

test/Analysis/CFNumber.c

21

What's "should work"? Is it "no warning is expected"?

[zaks.anna]

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

441

Since this a path-sensitive checker it could be a bit trickier to make sure that casts can be used for suppression; you'd need to look at the expression, not the type of the region. (Also, there is probably not much value into having this as a path-sensitive checker.)

[rgov]

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

566

Yes all the reporting code is just placeholders for now.

test/Analysis/CFNumber.c

21

Right, it should not warn.

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

void NilArgChecker::checkPostStmt(const ObjCDictionaryLiteral *DL,
   for (unsigned i = 0; i < NumOfElements; ++i) {
     ObjCDictionaryElement Element = DL->getKeyValueElement(i);
     warnIfNilExpr(Element.Key, "Dictionary key cannot be nil", C);
     warnIfNilExpr(Element.Value, "Dictionary value cannot be nil", C);
   }
 }
 
 //===----------------------------------------------------------------------===//
-// 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;
 
 private:
-  void EmitError(const TypedRegion* R, const Expr *Ex,
-                uint64_t SourceSize, uint64_t TargetSize, uint64_t NumberKind);
+  bool checkIntegralType(uint64_t Expected, QualType T, const CallExpr *CE,
+                         bool isCreate, CheckerContext &C) const;
+  bool checkSpecialType(uint64_t Expected, QualType T, const CallExpr *CE,
+                         bool isCreate, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
 enum CFNumberType {
   kCFNumberSInt8Type = 1,
   kCFNumberSInt16Type = 2,
   kCFNumberSInt32Type = 3,
   kCFNumberSInt64Type = 4,
   kCFNumberFloat32Type = 5,
   kCFNumberFloat64Type = 6,
   kCFNumberCharType = 7,
   kCFNumberShortType = 8,
   kCFNumberIntType = 9,
   kCFNumberLongType = 10,
   kCFNumberLongLongType = 11,
   kCFNumberFloatType = 12,
   kCFNumberDoubleType = 13,
   kCFNumberCFIndexType = 14,
   kCFNumberNSIntegerType = 15,
   kCFNumberCGFloatType = 16
 };
 
-static Optional<uint64_t> GetCFNumberSize(ASTContext &Ctx, uint64_t i) {
-  static const unsigned char FixedSize[] = { 8, 16, 32, 64, 32, 64 };
-
-  if (i < kCFNumberCharType)
-    return FixedSize[i-1];
-
-  QualType T;
-
-  switch (i) {
-    case kCFNumberCharType:     T = Ctx.CharTy;     break;
-    case kCFNumberShortType:    T = Ctx.ShortTy;    break;
-    case kCFNumberIntType:      T = Ctx.IntTy;      break;
-    case kCFNumberLongType:     T = Ctx.LongTy;     break;
-    case kCFNumberLongLongType: T = Ctx.LongLongTy; break;
-    case kCFNumberFloatType:    T = Ctx.FloatTy;    break;
-    case kCFNumberDoubleType:   T = Ctx.DoubleTy;   break;
-    case kCFNumberCFIndexType:
-    case kCFNumberNSIntegerType:
-    case kCFNumberCGFloatType:
-      // FIXME: We need a way to map from names to Type*.
-    default:
-      return None;
-  }
-
-  return Ctx.getTypeSize(T);
-}
-
 #if 0
 static const char* GetCFNumberTypeStr(uint64_t i) {
   static const char* Names[] = {
     "kCFNumberSInt8Type",
     "kCFNumberSInt16Type",
     "kCFNumberSInt32Type",
     "kCFNumberSInt64Type",
     "kCFNumberFloat32Type",
static const char* Names[] = {
     "kCFNumberNSIntegerType",
     "kCFNumberCGFloatType"
   };
 
   return i <= kCFNumberCGFloatType ? Names[i-1] : "Invalid CFNumberType";
 }
 #endif
 
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
                                          CheckerContext &C) const {
+  // This checker applies to CFNumberCreate and CFNumberGetValue, which have
+  // the same argument indices for the expected type and value pointer, so we
+  // can check them the same way.
+
   ProgramStateRef state = C.getState();
+  ASTContext &Ctx = C.getASTContext();
+  if (!ICreate) {
+    ICreate = &Ctx.Idents.get("CFNumberCreate");
+    IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+  }
+
+  // We limit ourselves to checking only CFNumberCreate and CFNumberGetValue
   const FunctionDecl *FD = C.getCalleeDecl(CE);
   if (!FD)
     return;
-
-  ASTContext &Ctx = C.getASTContext();
-  if (!II)
-    II = &Ctx.Idents.get("CFNumberCreate");
-
-  if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+  if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue))
+    return;
+  if (CE->getNumArgs() != 3)
     return;
 
-  // Get the value of the "theType" argument.
+  // Get the value of the "theType" argument, which is the expected type
   const LocationContext *LCtx = C.getLocationContext();
   SVal TheTypeVal = state->getSVal(CE->getArg(1), LCtx);
-
-  // FIXME: We really should allow ranges of valid theType values, and
-  //   bifurcate the state appropriately.
   Optional<nonloc::ConcreteInt> V = TheTypeVal.getAs<nonloc::ConcreteInt>();
   if (!V)
     return;
-
   uint64_t NumberKind = V->getValue().getLimitedValue();
-  Optional<uint64_t> OptTargetSize = GetCFNumberSize(Ctx, NumberKind);
-
-  // FIXME: In some cases we can emit an error.
-  if (!OptTargetSize)
-    return;
-
-  uint64_t TargetSize = *OptTargetSize;
 
-  // 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
-  // size of the type being created.
+  // Get the type of the value that is being passed
   SVal TheValueExpr = state->getSVal(CE->getArg(2), LCtx);
-
-  // FIXME: Eventually we should handle arbitrary locations.  We can do this
-  //  by having an enhanced memory model that does low-level typing.
   Optional<loc::MemRegionVal> LV = TheValueExpr.getAs<loc::MemRegionVal>();
   if (!LV)
     return;
 
+  // FIXME: If we strip casts, I think we break
+  //   CFNumberCreate(0, kCFNumberCFIndexType, (CFIndex *)&x)
   const TypedValueRegion* R = dyn_cast<TypedValueRegion>(LV->stripCasts());
   if (!R)
     return;
 
-  QualType T = Ctx.getCanonicalType(R->getValueType());
+  QualType T = R->getValueType();
 
-  // FIXME: If the pointee isn't an integer type, should we flag a warning?
-  //  People can do weird stuff with pointers.
+  // Call out to specialized routines for checking that the types work
+  bool isCreate = (FD->getIdentifier() == ICreate);
+  this->checkIntegralType(NumberKind, T, CE, isCreate, C) ||
+    this->checkSpecialType(NumberKind, T, CE, isCreate, C);
+}
 
-  if (!T->isIntegralOrEnumerationType())
-    return;
+bool CFNumberChecker::checkIntegralType(uint64_t Expected, QualType T,
+                                        const CallExpr *CE, bool isCreate,
+                                        CheckerContext &C) const {
+  ASTContext &Ctx = C.getASTContext();
+  QualType CanonT = Ctx.getCanonicalType(T);
 
-  uint64_t SourceSize = Ctx.getTypeSize(T);
+  QualType ET;
+  switch (Expected) {
+    case kCFNumberSInt8Type:    ET = Ctx.getIntTypeForBitwidth(8, true);  break;
+    case kCFNumberSInt16Type:   ET = Ctx.getIntTypeForBitwidth(16, true); break;
+    case kCFNumberSInt32Type:   ET = Ctx.getIntTypeForBitwidth(32, true); break;
+    case kCFNumberSInt64Type:   ET = Ctx.getIntTypeForBitwidth(64, true); break;
+    case kCFNumberCharType:     ET = Ctx.CharTy;     break;
+    case kCFNumberShortType:    ET = Ctx.ShortTy;    break;
+    case kCFNumberIntType:      ET = Ctx.IntTy;      break;
+    case kCFNumberLongType:     ET = Ctx.LongTy;     break;
+    case kCFNumberLongLongType: ET = Ctx.LongLongTy; break;
+    case kCFNumberFloatType:    ET = Ctx.FloatTy;    break;
+    case kCFNumberDoubleType:   ET = Ctx.DoubleTy;   break;
+    default:
+      return false;
+  }
 
-  // CHECK: is SourceSize == TargetSize
-  if (SourceSize == TargetSize)
-    return;
+  // If the types agree, then there is no problem
+  if (ET == CanonT)
+    return true;
 
-  // 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()
+  // Get the expected and actual sizes
+  uint64_t ExpectedSize = Ctx.getTypeSize(ET);
+  uint64_t SourceSize = Ctx.getTypeSize(T);
+  bool ExpectedSigned = ET->hasSignedIntegerRepresentation();
+  bool SourceSigned = T->hasSignedIntegerRepresentation();
+
+  // Report the error. We treat an out-of-bounds read as more important than a
+  // truncation.
+  ExplodedNode *N = SourceSize < ExpectedSize ? C.generateErrorNode()
                                               : C.generateNonFatalErrorNode();
-  if (N) {
-    SmallString<128> sbuf;
-    llvm::raw_svector_ostream os(sbuf);
+  if (!N)
+    return true;
+
 
-    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." ;
-    else
-      os << (SourceSize - TargetSize)
-      << " bits of the input integer will be lost.";
+  if (isCreate) {
+    os << "Creating a CFNumber from a value of expected type";
+  } else {
+    os << "Getting the value of a CFNumber with expected destination type";
+  }
+
+  os << " \"<description of type>\" but got \"<description of type>\"";
+
+  if (SourceSize < ExpectedSize)
+    os << " [overflow]";
+  else if (ExpectedSize < SourceSize)
+    os << " [truncation]";
+  else if (SourceSize == ExpectedSize && SourceSigned != ExpectedSigned)
+    os << " [signs differ]";
 
-    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());
-    C.emitReport(std::move(report));
+
+  return true;
+}
+
+static bool isTypedefOf(QualType T, StringRef Name) {
+  const TypedefType *TT = T->getAs<TypedefType>();
+  if (!TT)
+    return false;
+
+  if (TT->getDecl()->getName() == Name)
+    return true;
+
+  return isTypedefOf(TT->getDecl()->getUnderlyingType(), Name);
+}
+
+
+bool CFNumberChecker::checkSpecialType(uint64_t Expected, QualType T,
+                                       const CallExpr *CE, bool isCreate,
+                                       CheckerContext &C) const {
+  // Float64 "conforms to the 64-bit IEEE 754 standard"
+  ASTContext &Ctx = C.getASTContext();
+  QualType CanonT = Ctx.getCanonicalType(T);
+  if (Expected == kCFNumberFloat64Type && CanonT == Ctx.DoubleTy)
+    return true;
+
+  // Continue on to check that the name of the type is expected
+  const char *ExpectedName;
+  switch (Expected) {
+    case kCFNumberFloat32Type:   ExpectedName = "Float32";   break;
+    case kCFNumberFloat64Type:   ExpectedName = "Float64";   break;
+    case kCFNumberCFIndexType:   ExpectedName = "CFIndex";   break;
+    case kCFNumberNSIntegerType: ExpectedName = "NSInteger"; break;
+    case kCFNumberCGFloatType:   ExpectedName = "CGFloat";   break;
+    default:
+      return false;
   }
+
+  if (isTypedefOf(T, ExpectedName))
+    return true;
+
+  // Report the error
+  // XXX We could complain about overflow here too but it complicates the logic
+  ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+    return true;
+
+  SmallString<128> sbuf;
+  llvm::raw_svector_ostream os(sbuf);
+
+  if (isCreate) {
+    os << "Creating a CFNumber from a value of expected type";
+  } else {
+    os << "Getting the value of a CFNumber with expected destination type";
+  }
+
+  os << " \"" << ExpectedName << "\" but got \"<description of type>\"";
+
+  if (!BT)
+    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());
+  C.emitReport(std::move(report));
+
+  return true;
 }
 
 //===----------------------------------------------------------------------===//
 // CFRetain/CFRelease/CFMakeCollectable/CFAutorelease checking for null arguments.
 //===----------------------------------------------------------------------===//
 
 namespace {
 class CFRetainReleaseChecker : public Checker< check::PreStmt<CallExpr> > {

 //===----------------------------------------------------------------------===//
 // Check registration.
 //===----------------------------------------------------------------------===//
 
 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) {
   mgr.registerChecker<CFRetainReleaseChecker>();
 }
 
 void ento::registerClassReleaseChecker(CheckerManager &mgr) {
   mgr.registerChecker<ClassReleaseChecker>();

lib/StaticAnalyzer/Checkers/Checkers.td

 def DirectIvarAssignmentForAnnotatedFunctions : Checker<"DirectIvarAssignmentForAnnotatedFunctions">,
   HelpText<"Check for direct assignments to instance variables in the methods annotated with objc_no_direct_instance_variable_assignment">,
   DescFile<"DirectIvarAssignment.cpp">;
 
 } // end "alpha.osx.cocoa"
 
 let ParentPackage = CoreFoundation in {
 
-def CFNumberCreateChecker : Checker<"CFNumber">,
-  HelpText<"Check for proper uses of CFNumberCreate">,
+def CFNumberChecker : Checker<"CFNumber">,
+  HelpText<"Check for correct types passed to CFNumberCreate and CFNumberGetValue">,
   DescFile<"BasicObjCFoundationChecks.cpp">;
 
 def CFRetainReleaseChecker : Checker<"CFRetainRelease">,
   HelpText<"Check for null arguments to CFRetain/CFRelease/CFMakeCollectable">,
   DescFile<"BasicObjCFoundationChecks.cpp">;
 
 def CFErrorChecker : Checker<"CFError">,
   HelpText<"Check usage of CFErrorRef* parameters">,

test/Analysis/CFNumber.c

enum { kCFNumberSInt8Type = 1, kCFNumberSInt16Type = 2,
        kCFNumberLongLongType = 11, kCFNumberFloatType = 12,
        kCFNumberDoubleType = 13, kCFNumberCFIndexType = 14,
        kCFNumberNSIntegerType = 15, kCFNumberCGFloatType = 16,
        kCFNumberMaxType = 16 };
 typedef CFIndex CFNumberType;
 typedef const struct __CFNumber * CFNumberRef;
 extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const 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}}
+// Passing in a correctly-sized value
+__attribute__((cf_returns_retained))
+CFNumberRef f1(signed char x) {
+  return CFNumberCreate(0, kCFNumberSInt8Type, &x); /* should work */
 }
 
-__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}}
+// Passing in a correctly-sized value by a qualified pointer
+__attribute__((cf_returns_retained))
+CFNumberRef f2(const signed char* const x) {
+  return CFNumberCreate(0, kCFNumberSInt8Type, x); /* should work */
 }
 
-// test that the attribute overrides the naming convention.
-__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
-  return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
+// Passing an unsigned value where a signed one is expected
+__attribute__((cf_returns_retained))
+CFNumberRef f3(unsigned char x) {
+  return CFNumberCreate(0, kCFNumberSInt8Type, &x);
+}
+
+// Passing a value of a smaller type than is expected
+__attribute__((cf_returns_retained))
+CFNumberRef f4(char x) {
+  return CFNumberCreate(0, kCFNumberSInt16Type, &x);
+}
+
+// Passing a value of a larger type than is expected
+__attribute__((cf_returns_retained))
+CFNumberRef f5(unsigned int x) {
+  return CFNumberCreate(0, kCFNumberSInt8Type, &x);
+}
+
+// Passing a double in for a Float64 is OK
+__attribute__((cf_returns_retained))
+CFNumberRef f6(double x) {
+  return CFNumberCreate(0, kCFNumberFloat64Type, &x); /* should work */
+}
+
+// Passing a CFIndex when one is expected
+__attribute__((cf_returns_retained))
+CFNumberRef f7(CFIndex x) {
+  return CFNumberCreate(0, kCFNumberCFIndexType, &x); /* should work */
 }
 
-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}}
+// Passing a CFIndex by another name when one is expected
+typedef CFIndex _CFIndex;
+__attribute__((cf_returns_retained))
+CFNumberRef f8(_CFIndex x) {
+  return CFNumberCreate(0, kCFNumberCFIndexType, &x); /* should work */
+}
+
+// Passing in something other than a CFIndex when one is expected
+__attribute__((cf_returns_retained))
+CFNumberRef f9(signed long x) {
+  return CFNumberCreate(0, kCFNumberCFIndexType, &x);
+}
+
+// Passing in something other than a CFIndex when one is expected, even if they
+// have the same underlying type
+typedef long NotCFIndex;
+__attribute__((cf_returns_retained))
+CFNumberRef f10(NotCFIndex x) {
+  return CFNumberCreate(0, kCFNumberCFIndexType, &x);
+}
+
+// Passing in a value explicitly cast to a CFIndex when one is expected
+typedef long NotCFIndex;
+__attribute__((cf_returns_retained))
+CFNumberRef f11(NotCFIndex x) {
+  return CFNumberCreate(0, kCFNumberCFIndexType, (CFIndex *)&x); /* should work */
+}
+
+// Test that the attribute overrides the naming convention
+__attribute__((cf_returns_not_retained))
+CFNumberRef CreateNum(char x) {
+  return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
 }