[PATCH] Illegal delete/free not detected by clang static analyzer(MallocChecker)

[Karthik Bhat]

Hi All,
Yes Jordan as you pointed out in the current infrastructure doesn't allow
analyzer to conclude if a condition is true along all paths. I will have a
look into core if it is possible to achieve the same. Currently i have
backed out delete null case.

Added patch for "Argument to delete is uninitialized" . Added test case for
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,16 @@

 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_msg_undef;
   mutable OwningPtr<BugType> BT_objc_prop_undef;
   mutable OwningPtr<BugType> BT_objc_subscript_undef;
@@ -44,6 +47,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 +61,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 +257,32 @@
   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;
+  }
+}
+
+
 void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
                                          CheckerContext &C) const {
   ProgramStateRef State = C.getState();
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,26 @@
   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}}
+}
+


Regards
Karthik


On Tue, Aug 6, 2013 at 6:03 AM, Jordan Rose <jordan_rose at apple.com> wrote:

>
> On Aug 5, 2013, at 10:38 , John McCall <rjmccall at apple.com> wrote:
>
> On Aug 5, 2013, at 9:09 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> delete nullptr / delete 0 is entirely legal in C++. Can you please take
> that back out?
>
>
> An analysis which points out that what’s being deleted is *always* null
> would
> probably be useful, but I assume that would require different logic.
>
>
> Yes...unfortunately the analyzer is not great at proving something true
> along *all* paths that lead to a particular statement, instead of just *
> one* path.
>
> Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130806/3e35fc55/attachment.html>