[PATCH] Illegal delete/free not detected by clang static analyzer(MallocChecker)
Karthik Bhat
blitz.opensource at gmail.com
Mon Aug 5 02:47:56 PDT 2013
Hi Jordan,
Thanks for the inputs. I have modified the same in CallAndMessageChecker to
handle delete and delete[] cases.
Added cases for the same in NewDelete-checker-test.cpp.
Also there is one more issue not detected by new/delete checker.
For a TC like -
char* x = 0;
delete x;
we do not get Null dereference warning. Added patch to handle the same.
Please let me know your inputs on the same.
Index: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (revision 187716)
+++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (working copy)
@@ -28,13 +28,17 @@
namespace {
class CallAndMessageChecker
- : public Checker< check::PreStmt<CallExpr>, check::PreObjCMessage,
+ : public Checker< check::PreStmt<CallExpr>,
+ check::PreStmt<CXXDeleteExpr>,
+ check::PreObjCMessage,
check::PreCall > {
mutable OwningPtr<BugType> BT_call_null;
mutable OwningPtr<BugType> BT_call_undef;
mutable OwningPtr<BugType> BT_cxx_call_null;
mutable OwningPtr<BugType> BT_cxx_call_undef;
mutable OwningPtr<BugType> BT_call_arg;
+ mutable OwningPtr<BugType> BT_cxx_arg_undef;
+ mutable OwningPtr<BugType> BT_cxx_arg_null;
mutable OwningPtr<BugType> BT_msg_undef;
mutable OwningPtr<BugType> BT_objc_prop_undef;
mutable OwningPtr<BugType> BT_objc_subscript_undef;
@@ -44,6 +48,7 @@
public:
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C)
const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -57,6 +62,9 @@
void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg,
ExplodedNode *N) const;
+ void ReportBadDelete(const CXXDeleteExpr *DE, CheckerContext &C,
+ BugType *BT, StringRef Desc, ExplodedNode *N) const;
+
void HandleNilReceiver(CheckerContext &C,
ProgramStateRef state,
const ObjCMethodCall &msg) const;
@@ -250,6 +258,52 @@
C.addTransition(StNonNull);
}
+void CallAndMessageChecker::ReportBadDelete(const CXXDeleteExpr *DE,
+ CheckerContext &C,
+ BugType *BT,
+ StringRef Desc,
+ ExplodedNode *N) const {
+
+ BugReport *R = new BugReport(*BT, Desc, N);
+ bugreporter::trackNullOrUndefValue(N, DE, *R);
+ C.emitReport(R);
+}
+
+void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE,
+ CheckerContext &C) const {
+
+ SVal RHS = C.getSVal(DE->getArgument());
+ if (DE && RHS.isUndef()) {
+ ExplodedNode *N = C.generateSink();
+ if (!BT_cxx_arg_undef)
+ BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));
+ StringRef Desc = "Argument to delete is uninitialized";
+ ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);
+ return;
+ }
+
+ DefinedOrUnknownSVal location = RHS.castAs<DefinedOrUnknownSVal>();
+ // Check for null dereferences.
+ if (!location.getAs<Loc>())
+ return;
+
+ ProgramStateRef state = C.getState();
+
+ ProgramStateRef notNullState, nullState;
+ llvm::tie(notNullState, nullState) = state->assume(location);
+ if (nullState) {
+ if (!notNullState) {
+ ExplodedNode *N = C.generateSink();
+ if (!BT_cxx_arg_null)
+ BT_cxx_arg_null.reset(new BugType("Null Dereference", "Memory
Error"));
+ StringRef Desc = "Dereference of null pointer";
+ ReportBadDelete(DE,C,BT_cxx_arg_null.get(),Desc,N);
+ return;
+ }
+ }
+}
+
+
void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
Index: test/Analysis/inlining/containers.cpp
===================================================================
--- test/Analysis/inlining/containers.cpp (revision 187717)
+++ test/Analysis/inlining/containers.cpp (working copy)
@@ -100,7 +100,7 @@
#endif
}
- ~MySet() { delete[] storage; }
+ ~MySet() { if (storage) delete[] storage; }
bool isEmpty() {
clang_analyzer_checkInlined(true); // expected-warning {{TRUE}}
Index: test/Analysis/NewDelete-checker-test.cpp
===================================================================
--- test/Analysis/NewDelete-checker-test.cpp (revision 187717)
+++ test/Analysis/NewDelete-checker-test.cpp (working copy)
@@ -4,6 +4,7 @@
typedef __typeof__(sizeof(int)) size_t;
extern "C" void *malloc(size_t);
+extern "C" void free (void* ptr);
int *global;
//------------------
@@ -207,7 +208,50 @@
escapeVoidPtr(y);
} // no-warning
+//============== Test Uninitialized delete delete[]========================
+void testUninitDelete() {
+ int *x;
+ int * y = new int;
+ delete x; // expected-warning{{Argument to delete is uninitialized}}
+ delete y;
+}
+void testUninitDeleteArray() {
+ int *x;
+ int * y = new int[5];
+ delete[] x; // expected-warning{{Argument to delete is uninitialized}}
+ delete[] y;
+}
+
+void testUninitFree() {
+ int *x;
+ free(x); // expected-warning{{Function call argument is an uninitialized
value}}
+}
+
+
+//=============== Test for Null Dereference using
delete=======================
+void testNullDeref() {
+ int *x = 0;
+ delete x; // expected-warning{{Dereference of null pointer}}
+}
+
+void testArrayNullDeref() {
+ int *x = 0;
+ delete[] x; // expected-warning{{Dereference of null pointer}}
+}
+
+class derefTest {
+ int *storage;
+public:
+ derefTest() : storage(0) {};
+ ~derefTest(){delete[] storage;} // expected-warning{{Dereference of null
pointer}}
+};
+
+// Destructor call will result in a Null Dereference
+void testClassMemNullDeref() {
+ derefTest test;
+}
+
namespace reference_count {
class control_block {
unsigned count;
Regards
Karthik
On Fri, Aug 2, 2013 at 9:46 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> The real warning here isn't "Freeing non-allocated memory", it's "Argument
> to 'delete' is uninitialized". We already get this check for 'free' because
> it looks like a function call, but right now the same is not true for
> 'delete'.
>
> Eventually 'delete' will be modeled in terms of a destructor call and a
> deallocator, and at that point we'd probably get this warning for free. For
> now, though, this does seem useful.
>
> Please change the patch to only check 'delete' and 'delete[]', change the
> message to talk about uninitialized memory (and possibly use SVal::isUndef
> instead), and add tests for both 'delete' forms (plus a test for 'free' to
> make sure the call checker is still doing its work).
>
> Thanks for tracking this down!
> Jordan
>
>
> On Aug 2, 2013, at 3:06 , Karthik Bhat <blitz.opensource at gmail.com> wrote:
>
> > Hi,
> > In case we have a code like -
> >
> > int main() {
> > char* p;
> > delete p; // Illegal Delete
> > }
> >
> > Illegal delete for statement "delete p" is not reported. Added a patch
> to fix the same. Please let me know if the patch is ok.
> >
> > Index: test/Analysis/malloc-interprocedural.c
> > ===================================================================
> > --- test/Analysis/malloc-interprocedural.c (revision 187647)
> > +++ test/Analysis/malloc-interprocedural.c (working copy)
> > @@ -68,10 +68,13 @@
> > my_free1((int*)data); // expected-warning{{Use of memory after it is
> freed}}
> > }
> >
> > +static void my_free2(void *p) {
> > + free(p); // expected-warning{{Freeing a non allocated memory}}
> > +}
> > // TODO: We should warn here.
> > void test5() {
> > int *data;
> > - my_free1((int*)data);
> > + my_free2((int*)data);
> > }
> >
> > static char *reshape(char *in) {
> > Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> > ===================================================================
> > --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 187648)
> > +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)
> > @@ -961,8 +961,19 @@
> > bool ReturnsNullOnFailure)
> const {
> >
> > SVal ArgVal = State->getSVal(ArgExpr, C.getLocationContext());
> > - if (!ArgVal.getAs<DefinedOrUnknownSVal>())
> > + if (!ArgVal.getAs<DefinedOrUnknownSVal>()) {
> > + if (ExplodedNode *N = C.addTransition(C.getState())) {
> > + if (!BT_BadFree)
> > + BT_BadFree.reset(new BugType("Bad free", "Memory Error"));
> > + SmallString<100> buf;
> > + llvm::raw_svector_ostream os(buf);
> > + os << "Freeing a non allocated memory";
> > + BugReport *R = new BugReport(*BT_BadFree, os.str(), N);
> > + R->addRange(ArgExpr->getSourceRange());
> > + C.emitReport(R);
> > + }
> > return 0;
> > + }
> > DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>();
> >
> >
> > Thanks and Regards
> > Karthik Bhat
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130805/3da76323/attachment.html>
More information about the cfe-commits
mailing list