[cfe-commits] [patch] Catch a few simple misuses of free()
Jordy Rose
jediknil at belkadan.com
Fri Jun 4 22:48:03 PDT 2010
Changed:
- ignores unknown and undefined values.
- more comments about what's being tested
- messages that are hopefully more useful, which really is one of the best
things about Clang. I'm a little worried that the messages are a bit long
now.
- accepts HeapSpaceRegion. I'm still not sure this is a good idea --
things created by "new" shouldn't be free()d.
On Jun 3, 2010, at 8:28 PM, Jordy Rose wrote:
> Catch free()s on non-regions and regions known to be not from malloc()
(by
> checking the memory space).
-------------- next part --------------
Index: /Users/jordy/programming/llvm/tools/clang/test/Analysis/free.c
===================================================================
--- /Users/jordy/programming/llvm/tools/clang/test/Analysis/free.c (revision 0)
+++ /Users/jordy/programming/llvm/tools/clang/test/Analysis/free.c (revision 0)
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -fblocks -verify %s
+void free(void *);
+
+void t1 () {
+ int a[] = { 1 };
+ free(a); // expected-warning {{Argument to free() is the address of the local variable a, which is not memory allocated by malloc()}}
+}
+
+void t2 () {
+ int a = 1;
+ free(&a); // expected-warning {{Argument to free() is the address of the local variable a, which is not memory allocated by malloc()}}
+}
+
+void t3 () {
+ static int a[] = { 1 };
+ free(a); // expected-warning {{Argument to free() is the address of the static variable a, which is not memory allocated by malloc()}}
+}
+
+void t4 (char *x) {
+ free(x); // no-warning
+}
+
+void t5 () {
+ extern char *ptr();
+ free(ptr()); // no-warning
+}
+
+void t6 () {
+ free((void*)1000); // expected-warning {{Argument to free() is a constant address (1000), which is not memory allocated by malloc()}}
+}
+
+void t7 (char **x) {
+ free(*x); // no-warning
+}
+
+void t8 (char **x) {
+ // ugh
+ free((*x)+8); // no-warning
+}
+
+void t9 () {
+label:
+ free(&&label); // expected-warning {{Argument to free() is the address of the label 'label', which is not memory allocated by malloc()}}
+}
+
+void t10 () {
+ free((void*)&t10); // expected-warning {{Argument to free() is the address of the function t10, which is not memory allocated by malloc()}}
+}
+
+void t11 () {
+ char *p = (char*)__builtin_alloca(2);
+ free(p); // expected-warning {{Argument to free() was allocated by alloca(), not malloc()}}
+}
+
+void t12 () {
+ free(^{return;}); // expected-warning {{Argument to free() is a block, which is not memory allocated by malloc()}}
+}
+
+void t13 (char a) {
+ free(&a); // expected-warning {{Argument to free() is the address of the parameter a, which is not memory allocated by malloc()}}
+}
+
+static int someGlobal[2];
+void t14 () {
+ free(someGlobal); // expected-warning {{Argument to free() is the address of the global variable someGlobal, which is not memory allocated by malloc()}}
+}
+
+void t15 (char **x, int offset) {
+ // Unknown value
+ free(x[offset]); // no-warning
+}
Index: /Users/jordy/programming/llvm/tools/clang/lib/Checker/MallocChecker.cpp
===================================================================
--- /Users/jordy/programming/llvm/tools/clang/lib/Checker/MallocChecker.cpp (revision 105480)
+++ /Users/jordy/programming/llvm/tools/clang/lib/Checker/MallocChecker.cpp (working copy)
@@ -59,11 +59,12 @@
BuiltinBug *BT_DoubleFree;
BuiltinBug *BT_Leak;
BuiltinBug *BT_UseFree;
+ BuiltinBug *BT_BadFree;
IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc;
public:
MallocChecker()
- : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0),
+ : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_BadFree(0),
II_malloc(0), II_free(0), II_realloc(0), II_calloc(0) {}
static void *getTag();
bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
@@ -90,6 +91,10 @@
void ReallocMem(CheckerContext &C, const CallExpr *CE);
void CallocMem(CheckerContext &C, const CallExpr *CE);
+
+ bool SummarizeValue(llvm::raw_ostream& os, SVal V);
+ bool SummarizeRegion(llvm::raw_ostream& os, const MemRegion *MR);
+ void ReportBadFree(CheckerContext &C, SVal ArgVal, SourceRange range);
};
} // end anonymous namespace
@@ -191,18 +196,56 @@
const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE,
const GRState *state) {
- SVal ArgVal = state->getSVal(CE->getArg(0));
+ const Expr *ArgExpr = CE->getArg(0);
+ SVal ArgVal = state->getSVal(ArgExpr);
// If ptr is NULL, no operation is preformed.
if (ArgVal.isZeroConstant())
return state;
+
+ // Unknown values could easily be okay
+ // Undefined values are handled elsewhere
+ if (ArgVal.isUnknownOrUndef())
+ return state;
- SymbolRef Sym = ArgVal.getAsLocSymbol();
-
+ const MemRegion *R = ArgVal.getAsRegion();
+
+ // Nonlocs can't be freed, of course.
+ // Non-region locations (labels and fixed addresses) also shouldn't be freed.
+ if (!R) {
+ ReportBadFree(C, ArgVal, ArgExpr->getSourceRange());
+ return NULL;
+ }
+
+ R = R->StripCasts();
+
+ // Blocks might show up as heap data, but should not be free()d
+ if (isa<BlockDataRegion>(R)) {
+ ReportBadFree(C, ArgVal, ArgExpr->getSourceRange());
+ return NULL;
+ }
+
+ const MemSpaceRegion *MS = R->getMemorySpace();
+
+ // Parameters, locals, statics, and globals shouldn't be freed.
+ if (!(isa<UnknownSpaceRegion>(MS) || isa<HeapSpaceRegion>(MS))) {
+ // Conjured symbols are all unknown-space right now,
+ // but malloc() regions ought to be in HeapSpaceRegion.
+ // Of course, free() can work on memory allocated outside the current
+ // function, so UnknownSpaceRegion is always a possibility.
+
+ ReportBadFree(C, ArgVal, ArgExpr->getSourceRange());
+ return NULL;
+ }
+
+ const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R);
// Various cases could lead to non-symbol values here.
- if (!Sym)
+ // For now, ignore them.
+ if (!SR)
return state;
+ SymbolRef Sym = SR->getSymbol();
+
const RefState *RS = state->get<RegionState>(Sym);
// If the symbol has not been tracked, return. This is possible when free() is
@@ -230,6 +273,135 @@
return state->set<RegionState>(Sym, RefState::getReleased(CE));
}
+bool MallocChecker::SummarizeValue(llvm::raw_ostream& os, SVal V) {
+ if (nonloc::ConcreteInt *IntVal = dyn_cast<nonloc::ConcreteInt>(&V))
+ os << "an integer (" << IntVal->getValue() << ")";
+ else if (loc::ConcreteInt *ConstAddr = dyn_cast<loc::ConcreteInt>(&V))
+ os << "a constant address (" << ConstAddr->getValue() << ")";
+ else if (loc::GotoLabel *Label = dyn_cast<loc::GotoLabel>(&V))
+ os << "the address of the label '"
+ << Label->getLabel()->getID()->getName()
+ << "'";
+ else
+ return false;
+
+ return true;
+}
+
+bool MallocChecker::SummarizeRegion(llvm::raw_ostream& os,
+ const MemRegion *MR) {
+ switch (MR->getKind()) {
+ case MemRegion::FunctionTextRegionKind: {
+ const FunctionDecl *FD = cast<FunctionTextRegion>(MR)->getDecl();
+ if (FD)
+ os << "the address of the function " << FD;
+ else
+ os << "the address of a function";
+ return true;
+ }
+ case MemRegion::BlockTextRegionKind:
+ os << "block text";
+ return true;
+ case MemRegion::BlockDataRegionKind:
+ // FIXME: where the block came from?
+ os << "a block";
+ return true;
+ default: {
+ const MemSpaceRegion *MS = MR->getMemorySpace();
+
+ switch (MS->getKind()) {
+ case MemRegion::StackLocalsSpaceRegionKind: {
+ const VarRegion *VR = dyn_cast<VarRegion>(MR);
+ const VarDecl *VD;
+ if (VR)
+ VD = VR->getDecl();
+ else
+ VD = NULL;
+
+ if (VD)
+ os << "the address of the local variable " << VD->getName();
+ else
+ os << "the address of a local stack variable";
+ return true;
+ }
+ case MemRegion::StackArgumentsSpaceRegionKind: {
+ const VarRegion *VR = dyn_cast<VarRegion>(MR);
+ const VarDecl *VD;
+ if (VR)
+ VD = VR->getDecl();
+ else
+ VD = NULL;
+
+ if (VD)
+ os << "the address of the parameter " << VD->getName();
+ else
+ os << "the address of a parameter";
+ return true;
+ }
+ case MemRegion::GlobalsSpaceRegionKind: {
+ const VarRegion *VR = dyn_cast<VarRegion>(MR);
+ const VarDecl *VD;
+ if (VR)
+ VD = VR->getDecl();
+ else
+ VD = NULL;
+
+ if (VD) {
+ if (VD->isStaticLocal())
+ os << "the address of the static variable " << VD->getName();
+ else
+ os << "the address of the global variable " << VD->getName();
+ } else
+ os << "the address of a global variable";
+ return true;
+ }
+ default:
+ return false;
+ }
+ }
+ }
+}
+
+void MallocChecker::ReportBadFree(CheckerContext &C, SVal ArgVal,
+ SourceRange range) {
+ ExplodedNode *N = C.GenerateSink();
+ if (N) {
+ if (!BT_BadFree)
+ BT_BadFree = new BuiltinBug("Bad free");
+
+ llvm::SmallString<100> buf;
+ llvm::raw_svector_ostream os(buf);
+
+ const MemRegion *MR = ArgVal.getAsRegion();
+ if (MR) {
+ while (const ElementRegion *ER = dyn_cast<ElementRegion>(MR))
+ MR = ER->getSuperRegion();
+
+ // Special case for alloca()
+ if (isa<AllocaRegion>(MR))
+ os << "Argument to free() was allocated by alloca(), not malloc()";
+ else {
+ os << "Argument to free() is ";
+ if (SummarizeRegion(os, MR))
+ os << ", which is not memory allocated by malloc()";
+ else
+ os << "not memory allocated by malloc()";
+ }
+ } else {
+ os << "Argument to free() is ";
+ if (SummarizeValue(os, ArgVal))
+ os << ", which is not memory allocated by malloc()";
+ else
+ os << "not memory allocated by malloc()";
+ }
+
+ os.flush();
+ EnhancedBugReport *R = new EnhancedBugReport(*BT_BadFree, buf.str(), N);
+ R->addRange(range);
+ C.EmitReport(R);
+ }
+}
+
void MallocChecker::ReallocMem(CheckerContext &C, const CallExpr *CE) {
const GRState *state = C.getState();
const Expr *Arg0 = CE->getArg(0);
More information about the cfe-commits
mailing list