Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -336,21 +336,23 @@ } //===----------------------------------------------------------------------===// -// Error reporting. +// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue. //===----------------------------------------------------------------------===// namespace { -class CFNumberCreateChecker : public Checker< check::PreStmt > { +class CFNumberChecker : public Checker< check::PreStmt > { mutable std::unique_ptr 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 @@ -373,33 +375,6 @@ kCFNumberCGFloatType = 16 }; -static Optional 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[] = { @@ -425,100 +400,185 @@ } #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 V = TheTypeVal.getAs(); if (!V) return; - uint64_t NumberKind = V->getValue().getLimitedValue(); - Optional 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 LV = TheValueExpr.getAs(); if (!LV) return; + // FIXME: If we strip casts, I think we break + // CFNumberCreate(0, kCFNumberCFIndexType, (CFIndex *)&x) const TypedValueRegion* R = dyn_cast(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); + + 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; + } + // If the types agree, then there is no problem + if (ET == CanonT) + return true; + + // 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(); - // CHECK: is SourceSize == TargetSize - if (SourceSize == TargetSize) - return; + // 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) + 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() - : C.generateNonFatalErrorNode(); - if (N) { - SmallString<128> sbuf; - llvm::raw_svector_ostream os(sbuf); + 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 << (SourceSize == 8 ? "An " : "A ") - << SourceSize << " bit integer is used to initialize a CFNumber " - "object that represents " - << (TargetSize == 8 ? "an " : "a ") - << TargetSize << " bit integer. "; + os << " \"\" but got \"\""; - 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 (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")); + if (!BT) + BT.reset(new APIMisuse(this, "Bad use of CFNumber APIs")); - auto report = llvm::make_unique(*BT, os.str(), N); - report->addRange(CE->getArg(2)->getSourceRange()); - C.emitReport(std::move(report)); + auto report = llvm::make_unique(*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(); + 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 \"\""; + + if (!BT) + BT.reset(new APIMisuse(this, "Bad use of CFNumber APIs")); + + auto report = llvm::make_unique(*BT, os.str(), N); + report->addRange(CE->getArg(2)->getSourceRange()); + C.emitReport(std::move(report)); + + return true; } //===----------------------------------------------------------------------===// @@ -1272,8 +1332,8 @@ mgr.registerChecker(); } -void ento::registerCFNumberCreateChecker(CheckerManager &mgr) { - mgr.registerChecker(); +void ento::registerCFNumberChecker(CheckerManager &mgr) { + mgr.registerChecker(); } void ento::registerCFRetainReleaseChecker(CheckerManager &mgr) { Index: lib/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- lib/StaticAnalyzer/Checkers/Checkers.td +++ lib/StaticAnalyzer/Checkers/Checkers.td @@ -537,8 +537,8 @@ 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">, Index: test/Analysis/CFNumber.c =================================================================== --- test/Analysis/CFNumber.c +++ test/Analysis/CFNumber.c @@ -15,19 +15,78 @@ 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}} }