From: Anna Zaks Date: Mon, 30 Jan 2012 06:42:48 +0000 (+0000) Subject: [analyzer] Add index out of bounds check for CFArrayGetArrayAtIndex. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=af5f550de34525b27f0ff31dafce792caf8158b6;p=clang [analyzer] Add index out of bounds check for CFArrayGetArrayAtIndex. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@149228 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h index 8191ed1312..a3e1830be3 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h @@ -178,7 +178,8 @@ public: ProgramStateRef assumeInBound(DefinedOrUnknownSVal idx, DefinedOrUnknownSVal upperBound, - bool assumption) const; + bool assumption, + QualType IndexType = QualType()) const; /// Utility method for getting regions. const VarRegion* getRegion(const VarDecl *D, const LocationContext *LC) const; diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h index b80d494b88..e79b593584 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h @@ -9,8 +9,8 @@ // // This file defines partial implementations of template specializations of // the class ProgramStateTrait<>. ProgramStateTrait<> is used by ProgramState -// to implement set/get methods for mapulating a ProgramState's -// generic data map. +// to implement set/get methods for manipulating a ProgramState's +// generic data map. // //===----------------------------------------------------------------------===// diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 5c9b6bf1e3..9d628d40bb 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -44,6 +44,7 @@ add_clang_library(clangStaticAnalyzerCheckers OSAtomicChecker.cpp ObjCAtSyncChecker.cpp ObjCContainersASTChecker.cpp + ObjCContainersChecker.cpp ObjCSelfInitChecker.cpp ObjCUnusedIVarsChecker.cpp PointerArithChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/Checkers.td b/lib/StaticAnalyzer/Checkers/Checkers.td index 16fc53a241..f7c2a5735f 100644 --- a/lib/StaticAnalyzer/Checkers/Checkers.td +++ b/lib/StaticAnalyzer/Checkers/Checkers.td @@ -372,6 +372,10 @@ def ObjCContainersASTChecker : Checker<"ContainerAPI">, HelpText<"Check for common pitfalls when using 'CFArray', 'CFDictionary', 'CFSet' APIs">, DescFile<"ObjCContainersASTChecker.cpp">; +def ObjCContainersChecker : Checker<"Containers">, + HelpText<"Deep checks for common pitfalls when using 'CFArray' APIs">, + DescFile<"ObjCContainersChecker.cpp">; + def ObjCSelfInitChecker : Checker<"SelfInit">, HelpText<"Check that 'self' is properly initialized inside an initializer method">, DescFile<"ObjCSelfInitChecker.cpp">; diff --git a/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp new file mode 100644 index 0000000000..8c8020be5c --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp @@ -0,0 +1,161 @@ +//== ObjCContainersChecker.cpp - Path sensitive checker for CFArray *- C++ -*=// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// Performs path sensitive checks of Core Foundation static containers like +// CFArray. +// 1) Check for buffer overflows: +// In CFArrayGetArrayAtIndex( myArray, index), if the index is outside the +// index space of theArray (0 to N-1 inclusive (where N is the count of +// theArray), the behavior is undefined. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/AST/ParentMap.h" + +using namespace clang; +using namespace ento; + +namespace { +class ObjCContainersChecker : public Checker< check::PreStmt, + check::PostStmt > { + mutable llvm::OwningPtr BT; + inline void initBugType() const { + if (!BT) + BT.reset(new BugType("CFArray API", "Core Foundation/Objective-C API")); + } + + inline SymbolRef getArraySym(const Expr *E, CheckerContext &C) const { + SVal ArrayRef = C.getState()->getSVal(E, C.getLocationContext()); + SymbolRef ArraySym = ArrayRef.getAsSymbol(); + assert(ArraySym); + return ArraySym; + } + + void addSizeInfo(const Expr *Array, const Expr *Size, + CheckerContext &C) const; + +public: + /// A tag to id this checker. + static void *getTag() { static int Tag; return &Tag; } + + void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; + void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; +}; +} // end anonymous namespace + +// Array state stores the array size on creation (allocation). +// Size might be an unsigned or a symbol. +struct ArraySize { + SVal NLSize; + ArraySize(SVal s) : NLSize(s) {} + void Profile(llvm::FoldingSetNodeID &ID) const { + NLSize.Profile(ID); + } +}; + +// ProgramState trait - a map from array symbol to it's state. +typedef llvm::ImmutableMap ArraySizeM; + +namespace { struct ArraySizeMap {}; } +namespace clang { namespace ento { +template<> struct ProgramStateTrait + : public ProgramStatePartialTrait { + static void *GDMIndex() { return ObjCContainersChecker::getTag(); } +}; +}} + +void ObjCContainersChecker::addSizeInfo(const Expr *Array, const Expr *Size, + CheckerContext &C) const { + const ProgramState *State = C.getState(); + SVal SizeV = State->getSVal(Size, C.getLocationContext()); + // Undefined is reported by another checker. + if (SizeV.isUnknownOrUndef()) + return; + + // Get the ArrayRef symbol. + SVal ArrayRef = State->getSVal(Array, C.getLocationContext()); + SymbolRef ArraySym = ArrayRef.getAsSymbol(); + if (!ArraySym) + return; + + C.addTransition(State->set(ArraySym, SizeV)); + return; +} + +void ObjCContainersChecker::checkPostStmt(const CallExpr *CE, + CheckerContext &C) const { + StringRef Name = C.getCalleeName(CE); + if (Name.empty()) + return; + + // Add array size information to the state. + if (Name.equals("CFArrayCreate")) { + // Note, we can visit the Create method in the post-visit because + // the CFIndex parameter is passed in by value and will not be invalidated + // by the call. + addSizeInfo(CE, CE->getArg(2), C); + return; + } + + if (Name.equals("CFArrayGetCount")) { + addSizeInfo(CE->getArg(0), CE, C); + return; + } +} + +void ObjCContainersChecker::checkPreStmt(const CallExpr *CE, + CheckerContext &C) const { + StringRef Name = C.getCalleeName(CE); + if (Name.empty()) + return; + + // Check the array access. + if (Name.equals("CFArrayGetValueAtIndex")) { + const ProgramState *State = C.getState(); + // Retrieve the size. + // Find out if we saw this array symbol before and have information about it. + const Expr *ArrayExpr = CE->getArg(0); + const SVal *SizeVal = State->get(getArraySym(ArrayExpr, C)); + if (!SizeVal) + return; + DefinedOrUnknownSVal Size = cast(*SizeVal); + + // Get the index. + const Expr *IdxExpr = CE->getArg(1); + SVal IdxVal = State->getSVal(IdxExpr, C.getLocationContext()); + DefinedOrUnknownSVal Idx = cast(IdxVal); + + // Now, check if 'Idx in [0, Size-1]'. + const QualType T = IdxExpr->getType(); + const ProgramState *StInBound = State->assumeInBound(Idx, Size, true, T); + const ProgramState *StOutBound = State->assumeInBound(Idx, Size, false, T); + if (StOutBound && !StInBound) { + ExplodedNode *N = C.generateSink(StOutBound); + if (!N) + return; + initBugType(); + BugReport *R = new BugReport(*BT, "Index is out of bounds", N); + R->addRange(IdxExpr->getSourceRange()); + C.EmitReport(R); + return; + } + } +} + +/// Register checker. +void ento::registerObjCContainersChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} diff --git a/lib/StaticAnalyzer/Core/ProgramState.cpp b/lib/StaticAnalyzer/Core/ProgramState.cpp index 8c48a44073..b8b9ad94df 100644 --- a/lib/StaticAnalyzer/Core/ProgramState.cpp +++ b/lib/StaticAnalyzer/Core/ProgramState.cpp @@ -274,7 +274,8 @@ ProgramState::bindExprAndLocation(const Stmt *S, const LocationContext *LCtx, ProgramStateRef ProgramState::assumeInBound(DefinedOrUnknownSVal Idx, DefinedOrUnknownSVal UpperBound, - bool Assumption) const { + bool Assumption, + QualType indexTy) const { if (Idx.isUnknown() || UpperBound.isUnknown()) return this; @@ -288,7 +289,8 @@ ProgramStateRef ProgramState::assumeInBound(DefinedOrUnknownSVal Idx, // Get the offset: the minimum value of the array index type. BasicValueFactory &BVF = svalBuilder.getBasicValueFactory(); // FIXME: This should be using ValueManager::ArrayindexTy...somehow. - QualType indexTy = Ctx.IntTy; + if (indexTy.isNull()) + indexTy = Ctx.IntTy; nonloc::ConcreteInt Min(BVF.getMinValue(indexTy)); // Adjust the index. diff --git a/test/Analysis/CFContainers.mm b/test/Analysis/CFContainers.mm index ba2f386cf7..cead1d3ca9 100644 --- a/test/Analysis/CFContainers.mm +++ b/test/Analysis/CFContainers.mm @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=experimental.osx.cocoa.ContainerAPI -analyzer-store=region -triple x86_64-apple-darwin -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=experimental.osx.cocoa.ContainerAPI,experimental.osx.cocoa.Containers -analyzer-store=region -triple x86_64-apple-darwin -verify %s typedef const struct __CFAllocator * CFAllocatorRef; typedef const struct __CFString * CFStringRef; @@ -81,6 +81,10 @@ extern const CFSetCallBacks kCFTypeSetCallBacks; extern const CFDictionaryKeyCallBacks kCFCopyStringDictionaryKeyCallBacks; +extern +const void *CFArrayGetValueAtIndex(CFArrayRef theArray, CFIndex idx); +extern +CFIndex CFArrayGetCount(CFArrayRef theArray); CFDictionaryRef CFDictionaryCreate(CFAllocatorRef allocator, const void **keys, const void **values, CFIndex numValues, const CFDictionaryKeyCallBacks *keyCallBacks, const CFDictionaryValueCallBacks *valueCallBacks); CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr); @@ -123,4 +127,40 @@ void CreateDict(int *elems) { CFDictionaryRef dict1 = CFDictionaryCreate(kCFAllocatorDefault, (const void**)keys, (const void**)values, numValues, &keyCB, &valCB); // no warning CFDictionaryRef dict2 = CFDictionaryCreate(kCFAllocatorDefault, (const void**)elems[0], (const void**)values, numValues, &keyCB, &valCB); //expected-warning {{The first argument to 'CFDictionaryCreate' must be a C array of}} CFDictionaryRef dict3 = CFDictionaryCreate(kCFAllocatorDefault, (const void**)keys, (const void**)elems, numValues, &keyCB, &valCB); // expected-warning {{The second argument to 'CFDictionaryCreate' must be a C array of pointer-sized values}} -} \ No newline at end of file +} + +void OutOfBoundsSymbolicOffByOne(const void ** input, CFIndex S) { + CFArrayRef array; + array = CFArrayCreate(kCFAllocatorDefault, input, S, 0); + const void *s1 = CFArrayGetValueAtIndex(array, 0); // no warning + const void *s2 = CFArrayGetValueAtIndex(array, S-1); // no warning + const void *s3 = CFArrayGetValueAtIndex(array, S); // expected-warning {{Index is out of bounds}} +} + +void OutOfBoundsConst(const void ** input, CFIndex S) { + CFArrayRef array; + array = CFArrayCreate(kCFAllocatorDefault, input, 3, 0); + const void *s1 = CFArrayGetValueAtIndex(array, 0); // no warning + const void *s2 = CFArrayGetValueAtIndex(array, 2); // no warning + const void *s3 = CFArrayGetValueAtIndex(array, 5); // expected-warning {{Index is out of bounds}} + + // TODO: The solver is probably not strong enough here. + CFIndex sIndex; + for (sIndex = 0 ; sIndex <= 5 ; sIndex += 3 ) { + const void *s = CFArrayGetValueAtIndex(array, sIndex); + } +} + +void OutOfBoundsZiro(const void ** input, CFIndex S) { + CFArrayRef array; + // The API allows to set the size to 0. Check that we don't undeflow when the size is 0. + array = CFArrayCreate(kCFAllocatorDefault, 0, 0, 0); + const void *s1 = CFArrayGetValueAtIndex(array, 0); // expected-warning {{Index is out of bounds}} +} + +void TestGetCount(CFArrayRef A, CFIndex sIndex, CFIndex badIndex) { + CFIndex sCount = CFArrayGetCount(A); + if (sCount > sIndex) + const void *s1 = CFArrayGetValueAtIndex(A, sIndex); + const void *s2 = CFArrayGetValueAtIndex(A, sCount);// expected-warning {{Index is out of bounds}} +}