<div dir="ltr">Hi Jordan,<div>Thanks for the inputs. I have modified the same in CallAndMessageChecker to handle delete and delete[] cases. </div><div>Added cases for the same in NewDelete-checker-test.cpp.</div><div><br></div>
<div>Also there is one more issue not detected by new/delete checker. </div><div>For a TC like - <br></div><div>char* x = 0;<br></div><div>delete x; </div><div>we do not get Null dereference warning. Added patch to handle the same. Please let me know your inputs on the same.</div>
<div><br></div><div><br></div><div><br></div><div><br></div><div><div>Index: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp<span class="" style="white-space:pre"> </span>(revision 187716)</div>
<div>+++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -28,13 +28,17 @@</div><div> </div><div> namespace {</div><div> class CallAndMessageChecker</div>
<div>- : public Checker< check::PreStmt<CallExpr>, check::PreObjCMessage,</div><div>+ : public Checker< check::PreStmt<CallExpr>,</div><div>+ check::PreStmt<CXXDeleteExpr>,</div>
<div>+ check::PreObjCMessage,</div><div> check::PreCall > {</div><div> mutable OwningPtr<BugType> BT_call_null;</div><div> mutable OwningPtr<BugType> BT_call_undef;</div>
<div> mutable OwningPtr<BugType> BT_cxx_call_null;</div><div> mutable OwningPtr<BugType> BT_cxx_call_undef;</div><div> mutable OwningPtr<BugType> BT_call_arg;</div><div>+ mutable OwningPtr<BugType> BT_cxx_arg_undef;</div>
<div>+ mutable OwningPtr<BugType> BT_cxx_arg_null;</div><div> mutable OwningPtr<BugType> BT_msg_undef;</div><div> mutable OwningPtr<BugType> BT_objc_prop_undef;</div><div> mutable OwningPtr<BugType> BT_objc_subscript_undef;</div>
<div>@@ -44,6 +48,7 @@</div><div> public:</div><div> </div><div> void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;</div><div>+ void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;</div>
<div> void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const;</div><div> void checkPreCall(const CallEvent &Call, CheckerContext &C) const;</div><div> </div><div>@@ -57,6 +62,9 @@</div>
<div> void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg,</div><div> ExplodedNode *N) const;</div><div> </div><div>+ void ReportBadDelete(const CXXDeleteExpr *DE, CheckerContext &C,</div>
<div>+ BugType *BT, StringRef Desc, ExplodedNode *N) const;</div><div>+ </div><div> void HandleNilReceiver(CheckerContext &C,</div><div> ProgramStateRef state,</div><div>
const ObjCMethodCall &msg) const;</div><div>@@ -250,6 +258,52 @@</div><div> C.addTransition(StNonNull);</div><div> }</div><div> </div><div>+void CallAndMessageChecker::ReportBadDelete(const CXXDeleteExpr *DE, </div>
<div>+ CheckerContext &C,</div><div>+ BugType *BT,</div><div>+ StringRef Desc,</div><div>
+ ExplodedNode *N) const {</div><div>+</div><div>+ BugReport *R = new BugReport(*BT, Desc, N);</div><div>+ bugreporter::trackNullOrUndefValue(N, DE, *R);</div><div>+ C.emitReport(R);</div>
<div>+}</div><div>+</div><div>+void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE, </div><div>+ CheckerContext &C) const {</div><div>+</div><div>+ SVal RHS = C.getSVal(DE->getArgument());</div>
<div>+ if (DE && RHS.isUndef()) {</div><div>+ ExplodedNode *N = C.generateSink();</div><div>+ if (!BT_cxx_arg_undef)</div><div>+ BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));</div>
<div>+ StringRef Desc = "Argument to delete is uninitialized";</div><div>+ ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);</div><div>+ return;</div><div>+ }</div><div>+ </div><div>+ DefinedOrUnknownSVal location = RHS.castAs<DefinedOrUnknownSVal>();</div>
<div>+ // Check for null dereferences.</div><div>+ if (!location.getAs<Loc>())</div><div>+ return;</div><div>+</div><div>+ ProgramStateRef state = C.getState();</div><div>+</div><div>+ ProgramStateRef notNullState, nullState;</div>
<div>+ llvm::tie(notNullState, nullState) = state->assume(location);</div><div>+ if (nullState) {</div><div>+ if (!notNullState) {</div><div>+ ExplodedNode *N = C.generateSink();</div><div>+ if (!BT_cxx_arg_null)</div>
<div>+ BT_cxx_arg_null.reset(new BugType("Null Dereference", "Memory Error"));</div><div>+ StringRef Desc = "Dereference of null pointer";</div><div>+ ReportBadDelete(DE,C,BT_cxx_arg_null.get(),Desc,N);</div>
<div>+ return;</div><div>+ }</div><div>+ }</div><div>+}</div><div>+</div><div>+</div><div> void CallAndMessageChecker::checkPreCall(const CallEvent &Call,</div><div> CheckerContext &C) const {</div>
<div> ProgramStateRef State = C.getState();</div><div><br></div><div><br></div><div><br></div><div>Index: test/Analysis/inlining/containers.cpp</div><div>===================================================================</div>
<div>--- test/Analysis/inlining/containers.cpp<span class="" style="white-space:pre"> </span>(revision 187717)</div><div>+++ test/Analysis/inlining/containers.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -100,7 +100,7 @@</div><div> #endif</div><div> }</div><div> </div><div>- ~MySet() { delete[] storage; }</div><div>+ ~MySet() { if (storage) delete[] storage; }</div><div> </div><div> bool isEmpty() {</div><div>
clang_analyzer_checkInlined(true); // expected-warning {{TRUE}}</div><div><br></div><div><br></div><div><br></div><div>Index: test/Analysis/NewDelete-checker-test.cpp</div><div>===================================================================</div>
<div>--- test/Analysis/NewDelete-checker-test.cpp<span class="" style="white-space:pre"> </span>(revision 187717)</div><div>+++ test/Analysis/NewDelete-checker-test.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -4,6 +4,7 @@</div><div> </div><div> typedef __typeof__(sizeof(int)) size_t;</div><div> extern "C" void *malloc(size_t);</div><div>+extern "C" void free (void* ptr);</div><div> int *global;</div>
<div> </div><div> //------------------</div><div>@@ -207,7 +208,50 @@</div><div> escapeVoidPtr(y);</div><div> } // no-warning</div><div> </div><div>+//============== Test Uninitialized delete delete[]========================</div>
<div>+void testUninitDelete() {</div><div>+ int *x;</div><div>+ int * y = new int;</div><div>+ delete x; // expected-warning{{Argument to delete is uninitialized}}</div><div>+ delete y; </div><div>+}</div><div> </div>
<div>+void testUninitDeleteArray() {</div><div>+ int *x;</div><div>+ int * y = new int[5];</div><div>+ delete[] x; // expected-warning{{Argument to delete is uninitialized}}</div><div>+ delete[] y; </div><div>+}</div>
<div>+</div><div>+void testUninitFree() {</div><div>+ int *x;</div><div>+ free(x); // expected-warning{{Function call argument is an uninitialized value}}</div><div>+}</div><div>+</div><div>+</div><div>+//=============== Test for Null Dereference using delete=======================</div>
<div>+void testNullDeref() {</div><div>+ int *x = 0;</div><div>+ delete x; // expected-warning{{Dereference of null pointer}}</div><div>+}</div><div>+</div><div>+void testArrayNullDeref() {</div><div>+ int *x = 0;</div>
<div>+ delete[] x; // expected-warning{{Dereference of null pointer}}</div><div>+}</div><div>+</div><div>+class derefTest {</div><div>+ int *storage;</div><div>+public:</div><div>+ derefTest() : storage(0) {};</div><div>
+ ~derefTest(){delete[] storage;} // expected-warning{{Dereference of null pointer}}</div><div>+};</div><div>+</div><div>+// Destructor call will result in a Null Dereference</div><div>+void testClassMemNullDeref() {</div>
<div>+ derefTest test;</div><div>+}</div><div>+</div><div> namespace reference_count {</div><div> class control_block {</div><div> unsigned count;</div></div><div><br></div><div><br></div><div><br></div><div><br></div>
<div>Regards</div><div>Karthik</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 2, 2013 at 9:46 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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'.<br>
<br>
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.<br>
<br>
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).<br>
<br>
Thanks for tracking this down!<br>
Jordan<br>
<div><div class="h5"><br>
<br>
On Aug 2, 2013, at 3:06 , Karthik Bhat <<a href="mailto:blitz.opensource@gmail.com">blitz.opensource@gmail.com</a>> wrote:<br>
<br>
> Hi,<br>
> In case we have a code like -<br>
><br>
> int main() {<br>
> char* p;<br>
> delete p; // Illegal Delete<br>
> }<br>
><br>
> 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.<br>
><br>
> Index: test/Analysis/malloc-interprocedural.c<br>
> ===================================================================<br>
> --- test/Analysis/malloc-interprocedural.c (revision 187647)<br>
> +++ test/Analysis/malloc-interprocedural.c (working copy)<br>
> @@ -68,10 +68,13 @@<br>
> my_free1((int*)data); // expected-warning{{Use of memory after it is freed}}<br>
> }<br>
><br>
> +static void my_free2(void *p) {<br>
> + free(p); // expected-warning{{Freeing a non allocated memory}}<br>
> +}<br>
> // TODO: We should warn here.<br>
> void test5() {<br>
> int *data;<br>
> - my_free1((int*)data);<br>
> + my_free2((int*)data);<br>
> }<br>
><br>
> static char *reshape(char *in) {<br>
> Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>
> ===================================================================<br>
> --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 187648)<br>
> +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)<br>
> @@ -961,8 +961,19 @@<br>
> bool ReturnsNullOnFailure) const {<br>
><br>
> SVal ArgVal = State->getSVal(ArgExpr, C.getLocationContext());<br>
> - if (!ArgVal.getAs<DefinedOrUnknownSVal>())<br>
> + if (!ArgVal.getAs<DefinedOrUnknownSVal>()) {<br>
> + if (ExplodedNode *N = C.addTransition(C.getState())) {<br>
> + if (!BT_BadFree)<br>
> + BT_BadFree.reset(new BugType("Bad free", "Memory Error"));<br>
> + SmallString<100> buf;<br>
> + llvm::raw_svector_ostream os(buf);<br>
> + os << "Freeing a non allocated memory";<br>
> + BugReport *R = new BugReport(*BT_BadFree, os.str(), N);<br>
> + R->addRange(ArgExpr->getSourceRange());<br>
> + C.emitReport(R);<br>
> + }<br>
> return 0;<br>
> + }<br>
> DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>();<br>
><br>
><br>
> Thanks and Regards<br>
> Karthik Bhat<br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div><br></div>