[cfe-commits] [PATCH] Fix for PR7218, and analyzer support for calloc()
Jordy Rose
jediknil at belkadan.com
Fri May 28 17:28:09 PDT 2010
Ah, right. That would be necessary, huh. Thanks.
Jordy
On Fri, 28 May 2010 17:20:36 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> Hi Jordy,
>
> I don't see your patch. Did you forget to attach it?
>
> Ted
>
> On May 28, 2010, at 5:11 PM, Jordy Rose wrote:
>
>>
>> Fixes PR7218 "Assigning to buf[0] makes buf[1] valid" by not allowing
>> arrays and symbolic regions to have direct bindings, only bindings to
>> element 0. (The exception is symbolic regions for references, since
they
>> don't have elements.)
>>
>> On top of that, adds support for calloc(), treating its result as a
>> malloc-region that needs freeing, whose contents are initialized to 0.
>>
>> I'm not entirely happy with the replacement of *p with p[0] in
>> GRExprEngine, but that seems to be the last place where you can tell
the
>> difference between a binding to p and a binding to p[0].
>>
>> This could be split into two patches, of course, though I worked on it
>> all
>> simultaneously. The calloc() part requires a way to set default
elements
>> for regions -- I did it by using the assumption that arrays and
symbolic
>> regions couldn't get direct bindings, only default ones, but it could
>> also
>> be done by adding another method to Store (really RegionStore).
>>
>> Jordy
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
Index: test/Analysis/undef-buffers.c
===================================================================
--- test/Analysis/undef-buffers.c (revision 0)
+++ test/Analysis/undef-buffers.c (revision 0)
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-experimental-checks -analyzer-store=region -verify %s
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void free(void *);
+
+char stackBased1 () {
+ char buf[2];
+ buf[0] = 'a';
+ return buf[1]; // expected-warning{{Undefined}}
+}
+
+char stackBased2 () {
+ char buf[2];
+ buf[1] = 'a';
+ return buf[0]; // expected-warning{{Undefined}}
+}
+
+char heapBased1 () {
+ char *buf = malloc(2);
+ buf[0] = 'a';
+ char result = buf[1]; // expected-warning{{undefined}}
+ free(buf);
+ return result;
+}
+
+char heapBased2 () {
+ char *buf = malloc(2);
+ buf[1] = 'a';
+ char result = buf[0]; // expected-warning{{undefined}}
+ free(buf);
+ return result;
+}
Index: test/Analysis/malloc.c
===================================================================
--- test/Analysis/malloc.c (revision 104942)
+++ test/Analysis/malloc.c (working copy)
@@ -77,3 +77,36 @@
buf[1] = 'c'; // not crash
}
+
+// This tests that malloc() buffers are undefined by default
+char mallocGarbage () {
+ char *buf = malloc(2);
+ char result = buf[1]; // expected-warning{{undefined}}
+ free(buf);
+ return result;
+}
+
+// This tests that calloc() buffers need to be freed
+void callocNoFree () {
+ char *buf = calloc(2,2);
+ return; // expected-warning{{never released}}
+}
+
+// These test that calloc() buffers are zeroed by default
+char callocZeroesGood () {
+ char *buf = calloc(2,2);
+ char result = buf[3]; // no-warning
+ if (buf[1] == 0) {
+ free(buf);
+ }
+ return result; // no-warning
+}
+
+char callocZeroesBad () {
+ char *buf = calloc(2,2);
+ char result = buf[3]; // no-warning
+ if (buf[1] != 0) {
+ free(buf);
+ }
+ return result; // expected-warning{{never released}}
+}
Index: test/Analysis/outofbound.c
===================================================================
--- test/Analysis/outofbound.c (revision 104942)
+++ test/Analysis/outofbound.c (working copy)
@@ -2,6 +2,7 @@
typedef __typeof(sizeof(int)) size_t;
void *malloc(size_t);
+void *calloc(size_t, size_t);
char f1() {
char* s = "abcd";
@@ -36,3 +37,9 @@
p[1] = a; // no-warning
p[2] = a; // expected-warning{{Access out-of-bound array element (buffer overflow)}}
}
+
+void f5() {
+ char *p = calloc(2,2);
+ p[3] = '.'; // no-warning
+ p[4] = '!'; // expected-warning{{out-of-bound}}
+}
Index: lib/Checker/MallocChecker.cpp
===================================================================
--- lib/Checker/MallocChecker.cpp (revision 104942)
+++ lib/Checker/MallocChecker.cpp (working copy)
@@ -59,12 +59,12 @@
BuiltinBug *BT_DoubleFree;
BuiltinBug *BT_Leak;
BuiltinBug *BT_UseFree;
- IdentifierInfo *II_malloc, *II_free, *II_realloc;
+ IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc;
public:
MallocChecker()
: BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0),
- II_malloc(0), II_free(0), II_realloc(0) {}
+ II_malloc(0), II_free(0), II_realloc(0), II_calloc(0) {}
static void *getTag();
bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
void EvalDeadSymbols(CheckerContext &C,const Stmt *S,SymbolReaper &SymReaper);
@@ -76,12 +76,19 @@
private:
void MallocMem(CheckerContext &C, const CallExpr *CE);
const GRState *MallocMemAux(CheckerContext &C, const CallExpr *CE,
- const Expr *SizeEx, const GRState *state);
+ const Expr *SizeEx, SVal Init,
+ const GRState *state) {
+ return MallocMemAux(C, CE, state->getSVal(SizeEx), Init, state);
+ }
+ const GRState *MallocMemAux(CheckerContext &C, const CallExpr *CE,
+ SVal SizeEx, SVal Init,
+ const GRState *state);
void FreeMem(CheckerContext &C, const CallExpr *CE);
const GRState *FreeMemAux(CheckerContext &C, const CallExpr *CE,
const GRState *state);
void ReallocMem(CheckerContext &C, const CallExpr *CE);
+ void CallocMem(CheckerContext &C, const CallExpr *CE);
};
} // end anonymous namespace
@@ -120,6 +127,8 @@
II_free = &Ctx.Idents.get("free");
if (!II_realloc)
II_realloc = &Ctx.Idents.get("realloc");
+ if (!II_calloc)
+ II_calloc = &Ctx.Idents.get("calloc");
if (FD->getIdentifier() == II_malloc) {
MallocMem(C, CE);
@@ -135,29 +144,34 @@
ReallocMem(C, CE);
return true;
}
+
+ if (FD->getIdentifier() == II_calloc) {
+ CallocMem(C, CE);
+ return true;
+ }
return false;
}
void MallocChecker::MallocMem(CheckerContext &C, const CallExpr *CE) {
- const GRState *state = MallocMemAux(C, CE, CE->getArg(0), C.getState());
+ const GRState *state = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(),
+ C.getState());
C.addTransition(state);
}
const GRState *MallocChecker::MallocMemAux(CheckerContext &C,
const CallExpr *CE,
- const Expr *SizeEx,
+ SVal Size,
+ SVal Init,
const GRState *state) {
unsigned Count = C.getNodeBuilder().getCurrentBlockCount();
ValueManager &ValMgr = C.getValueManager();
SVal RetVal = ValMgr.getConjuredSymbolVal(NULL, CE, CE->getType(), Count);
- SVal Size = state->getSVal(SizeEx);
-
state = C.getEngine().getStoreManager().setExtent(state, RetVal.getAsRegion(),
Size);
-
+ state = state->bindLoc(RetVal, Init);
state = state->BindExpr(CE, RetVal);
SymbolRef Sym = RetVal.getAsLocSymbol();
@@ -234,7 +248,8 @@
if (Sym)
stateEqual = stateEqual->set<RegionState>(Sym, RefState::getReleased(CE));
- const GRState *stateMalloc = MallocMemAux(C, CE, CE->getArg(1), stateEqual);
+ const GRState *stateMalloc = MallocMemAux(C, CE, CE->getArg(1),
+ UndefinedVal(), stateEqual);
C.addTransition(stateMalloc);
}
@@ -256,13 +271,30 @@
if (stateFree) {
// FIXME: We should copy the content of the original buffer.
const GRState *stateRealloc = MallocMemAux(C, CE, CE->getArg(1),
- stateFree);
+ UnknownVal(), stateFree);
C.addTransition(stateRealloc);
}
}
}
}
+void MallocChecker::CallocMem(CheckerContext &C, const CallExpr *CE) {
+ const GRState *state = C.getState();
+
+ ValueManager &ValMgr = C.getValueManager();
+ SValuator &SVator = C.getSValuator();
+
+ SVal Count = state->getSVal(CE->getArg(0));
+ SVal EleSize = state->getSVal(CE->getArg(1));
+ SVal TotalSize = SVator.EvalBinOp(state, BinaryOperator::Mul, Count, EleSize,
+ ValMgr.getContext().getSizeType());
+
+ SVal Zero = ValMgr.makeZeroVal(ValMgr.getContext().CharTy);
+
+ state = MallocMemAux(C, CE, TotalSize, Zero, state);
+ C.addTransition(state);
+}
+
void MallocChecker::EvalDeadSymbols(CheckerContext &C, const Stmt *S,
SymbolReaper &SymReaper) {
for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(),
Index: lib/Checker/RegionStore.cpp
===================================================================
--- lib/Checker/RegionStore.cpp (revision 104942)
+++ lib/Checker/RegionStore.cpp (working copy)
@@ -218,6 +218,11 @@
/// getDefaultBinding - Returns an SVal* representing an optional default
/// binding associated with a region and its subregions.
Optional<SVal> getDefaultBinding(RegionBindings B, const MemRegion *R);
+
+ /// canHaveDirectBinding - Disallow direct bindings for certain types,
+ /// like arrays. This lets us distinguish between x and x[0], which was
+ /// causing PR7218 "Assigning to buf[0] makes buf[1] valid".
+ bool canHaveDirectBinding (const MemRegion *R);
/// setImplicitDefaultValue - Set the default binding for the provided
/// MemRegion to the value implicitly defined for compound literals when
@@ -945,10 +950,25 @@
// Loading values from regions.
//===----------------------------------------------------------------------===//
+bool RegionStoreManager::canHaveDirectBinding (const MemRegion *R) {
+ // Arrays can't have direct binding -- must bind to elements
+ if (const TypedRegion *TR = dyn_cast<TypedRegion>(R))
+ if (TR->getValueType(getContext())->isArrayType())
+ return false;
+
+ // Symbolic regions are the same, unless they are references
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
+ if (!SR->getSymbol()->getType(getContext())->isReferenceType())
+ return false;
+
+ return true;
+}
+
Optional<SVal> RegionStoreManager::getDirectBinding(RegionBindings B,
const MemRegion *R) {
- if (const SVal *V = Lookup(B, R, BindingKey::Direct))
- return *V;
+ if (canHaveDirectBinding(R))
+ if (const SVal *V = Lookup(B, R, BindingKey::Direct))
+ return *V;
return Optional<SVal>();
}
@@ -1089,7 +1109,7 @@
}
RegionBindings B = GetRegionBindings(store);
- const SVal *V = Lookup(B, R, BindingKey::Direct);
+ Optional<SVal> V = getBinding(B, R);
// Check if the region has a binding.
if (V)
@@ -1226,7 +1246,7 @@
if (D->isZeroConstant())
return ValMgr.makeZeroVal(Ty);
- if (D->isUnknown())
+ if (D->isUnknownOrUndef())
return *D;
assert(0 && "Unknown default value");
@@ -1383,6 +1403,8 @@
if (TR->getValueType(getContext())->isStructureOrClassType())
return BindStruct(store, TR, V);
+ BindingKey::Kind BindingKind = BindingKey::Direct;
+
// Special case: the current region represents a cast and it and the super
// region both have pointer types or intptr_t types. If so, perform the
// bind to the super region.
@@ -1408,23 +1430,15 @@
}
}
}
- else if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
- // Binding directly to a symbolic region should be treated as binding
- // to element 0.
- QualType T = SR->getSymbol()->getType(getContext());
-
- // FIXME: Is this the right way to handle symbols that are references?
- if (const PointerType *PT = T->getAs<PointerType>())
- T = PT->getPointeeType();
- else
- T = T->getAs<ReferenceType>()->getPointeeType();
-
- R = GetElementZeroRegion(SR, T);
+ else if (!canHaveDirectBinding(R)) {
+ // Binding directly to, say, an array or symbolic region
+ // should be treated as providing a default value
+ BindingKind = BindingKey::Default;
}
// Perform the binding.
RegionBindings B = GetRegionBindings(store);
- return Add(B, R, BindingKey::Direct, V).getRoot();
+ return Add(B, R, BindingKind, V).getRoot();
}
Store RegionStoreManager::BindDecl(Store store, const VarRegion *VR,
Index: lib/Checker/GRExprEngine.cpp
===================================================================
--- lib/Checker/GRExprEngine.cpp (revision 104942)
+++ lib/Checker/GRExprEngine.cpp (working copy)
@@ -2675,11 +2675,22 @@
const GRState* state = GetState(*I);
SVal location = state->getSVal(Ex);
- if (asLValue)
+ if (asLValue) {
+ // When treating a symbolic region as an l-value,
+ // do it as an element reference, in order not to confuse the Store
+ if (const MemRegion *MR = location.getAsRegion()) {
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR)) {
+ const ElementRegion *ER =
+ getStoreManager().GetElementZeroRegion(SR, Ex->getType());
+ location = loc::MemRegionVal(ER);
+ }
+ }
+
MakeNode(Dst, U, *I, state->BindExpr(U, location),
ProgramPoint::PostLValueKind);
- else
+ } else {
EvalLoad(Dst, U, *I, state, location);
+ }
}
return;
More information about the cfe-commits
mailing list