[analyzer] Report CFNumberGetValue API misuse

[zaks.anna]

Summary

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.

[dcoughlin]

test/Analysis/CFNumber.c

39

I feel like this diagnostic is not dire enough. The problem is not that the bits will be lost -- but rather that they will be overwritten on top of something else!

How about something like "The remaining 8 bits will overwrite adjacent storage."?

45

Some grammar/style nits. (I realize these also hold for the pre-existing checks):

• There should be a dash between the number and 'bit'. That is, it should be "16-bit" and "8-bit".
• It is generally considered bad style to start a sentence with a number that is not written out (except for dates). How about starting the second sentence with "The remaining 8 bits ..."?

[zaks.anna]

Address comments from Devin.

[dcoughlin]

LGTM!

[NoQ]

test/Analysis/CFNumber.c

40

We're not sure from this code if the CFNumber object x actually represents a 16-bit integer, or somebody just misplaced the kCFNumberSInt16Type thing. I think the warning message could be made more precise in this sence, but i'm not good at coming up with warning messages.

Hmm, there could actually be a separate check for detecting inconsistent type specifiers used for accessing the same CFNumber object.

[zaks.anna]

test/Analysis/CFNumber.c

40

I see your point. Looks like we'd need to modify both first part of the sentence and the second to address this concern. We could do something like "A CFNumber object treated as if it represents a 16-bit integer is used to initialize an 8-bit integer; 8 bits of the CFNumber value or the adjacent storage will overwrite adjacent storage of the integer".

Though this is more correct, I do not think it's worth the new language complexity. Also, the warning message is already quite long.

include/clang/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 proper uses of CFNumber APIs">,
   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">,

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);
 };
 } // end anonymous namespace
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 {
   ProgramStateRef state = C.getState();
   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 (!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.
   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);
+  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
   // size of the type being created.
   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;
 
   const TypedValueRegion* R = dyn_cast<TypedValueRegion>(LV->stripCasts());
   if (!R)
     return;
 
   QualType T = Ctx.getCanonicalType(R->getValueType());
 
   // FIXME: If the pointee isn't an integer type, should we flag a warning?
   //  People can do weird stuff with pointers.
 
   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 be "
+      << (isCreate ? "garbage." : "lost.");
     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());
     C.emitReport(std::move(report));
   }
 }
 
 //===----------------------------------------------------------------------===//

 //===----------------------------------------------------------------------===//
 // 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>();

test/Analysis/CFNumber.c

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.coreFoundation.CFNumber,osx.cocoa.RetainCount -analyzer-store=region -analyzer-constraints=range -verify -triple x86_64-apple-darwin9 %s
 
 typedef signed long CFIndex;
 typedef const struct __CFAllocator * CFAllocatorRef;
 enum { 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,
        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) {
+__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.
 __attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
   return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
 }
 
-CFNumberRef f3(unsigned i) {
+__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 be lost}}
+  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;
+}