Skip to content

Commit

Permalink
[analyzer] Report CFNumberGetValue API misuse
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@AnnaZaks
AnnaZaks committed Oct 26, 2016
1 parent 31d8b7d commit 5b2b390
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 42 deletions.
4 changes: 2 additions & 2 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View file
Expand Up @@ -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">,
Expand Down
77 changes: 42 additions & 35 deletions clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -425,18 +425,20 @@ 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);
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.
Expand All @@ -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
Expand All @@ -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());
Expand Down Expand Up @@ -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) {
Expand Down
24 changes: 19 additions & 5 deletions clang/test/Analysis/CFNumber.c
View file
Expand Up @@ -13,21 +13,35 @@ 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.
__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;
}

0 comments on commit 5b2b390

Please sign in to comment.