From 5b2b39065ca6369280f4f07d3dc42f0785721911 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Wed, 26 Oct 2016 22:51:47 +0000 Subject: [PATCH] [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 llvm-svn: 285253 --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 +- .../Checkers/BasicObjCFoundationChecks.cpp | 77 ++++++++++++---------- clang/test/Analysis/CFNumber.c | 24 +++++-- 3 files changed, 63 insertions(+), 42 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index f1f0d4b..16ccea6 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/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/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index 6239c55..1ea85d6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/clang/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 > { +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; @@ -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 OptTargetSize = GetCFNumberSize(Ctx, NumberKind); + Optional 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(*BT, os.str(), N); report->addRange(CE->getArg(2)->getSourceRange()); @@ -1272,8 +1279,8 @@ void ento::registerNilArgChecker(CheckerManager &mgr) { mgr.registerChecker(); } -void ento::registerCFNumberCreateChecker(CheckerManager &mgr) { - mgr.registerChecker(); +void ento::registerCFNumberChecker(CheckerManager &mgr) { + mgr.registerChecker(); } void ento::registerCFRetainReleaseChecker(CheckerManager &mgr) { diff --git a/clang/test/Analysis/CFNumber.c b/clang/test/Analysis/CFNumber.c index ebb3b1a..ec966ee 100644 --- a/clang/test/Analysis/CFNumber.c +++ b/clang/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; } -- 2.7.4