<div dir="ltr">Thanks Jordan for the inputs. Please find the patch with review comments -<div><br><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,16 @@</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_delete_undef;</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 +47,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>@@ -250,6 +254,34 @@</div>
<div> C.addTransition(StNonNull);</div><div> }</div><div> </div><div>+void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE,</div><div>+ CheckerContext &C) const {</div>
<div>+</div><div>+ SVal Arg = C.getSVal(DE->getArgument());</div><div>+ FunctionDecl* FD = DE->getOperatorDelete();</div><div>+ if (!FD)</div><div>+ return;</div><div>+ if (Arg.isUndef()) {</div><div>+ StringRef Desc;</div>
<div>+ OverloadedOperatorKind Kind = FD->getOverloadedOperator();</div><div>+ ExplodedNode *N = C.generateSink();</div><div>+ if (!N)</div><div>+ return;</div><div>+ if (!BT_cxx_delete_undef)</div><div>
+ BT_cxx_delete_undef.reset(new BuiltinBug("Uninitialized argument value"));</div><div>+ if (Kind == OO_Array_Delete)</div><div>+ Desc = "Argument to delete[] is uninitialized";</div><div>
+ else</div><div>+ Desc = "Argument to delete is uninitialized"; </div><div>+ BugType *BT = BT_cxx_delete_undef.get();</div><div>+ BugReport *R = new BugReport(*BT, Desc, N);</div><div>+ bugreporter::trackNullOrUndefValue(N, DE, *R);</div>
<div>+ C.emitReport(R);</div><div>+ return;</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>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,38 @@</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>+void testUninitDeleteSink() {</div><div>+ int *x;</div><div>+ delete x; // expected-warning{{Argument to delete is uninitialized}}</div><div>+ (*(volatile int *)0 = 1); // no warn</div><div>+}</div><div>+</div><div>
+void testUninitDeleteArraySink() {</div><div>+ int *x;</div><div>+ delete[] x; // expected-warning{{Argument to delete[] is uninitialized}}</div><div>+ (*(volatile int *)0 = 1); // no warn</div><div>+}</div><div>+</div>
<div> namespace reference_count {</div><div> class control_block {</div><div> unsigned count;</div><div><br></div><div><br></div></div><div><br></div><div>Regards</div><div>Karthik Bhat</div></div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Aug 6, 2013 at 9:27 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">
Looking good! Some inline comments on the patch itself, now.<br>
<div class="im"><br>
On Aug 5, 2013, at 21:16 , Karthik Bhat <<a href="mailto:blitz.opensource@gmail.com">blitz.opensource@gmail.com</a>> wrote:<br>
<br>
> Hi All,<br>
> 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.<br>
><br>
> Added patch for "Argument to delete is uninitialized" . Added test case for the same.<br>
><br>
><br>
> Index: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp<br>
> ===================================================================<br>
> --- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (revision 187716)<br>
> +++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (working copy)<br>
> @@ -28,13 +28,16 @@<br>
><br>
> namespace {<br>
> class CallAndMessageChecker<br>
> - : public Checker< check::PreStmt<CallExpr>, check::PreObjCMessage,<br>
> + : public Checker< check::PreStmt<CallExpr>,<br>
> + check::PreStmt<CXXDeleteExpr>,<br>
> + check::PreObjCMessage,<br>
> check::PreCall > {<br>
> mutable OwningPtr<BugType> BT_call_null;<br>
> mutable OwningPtr<BugType> BT_call_undef;<br>
> mutable OwningPtr<BugType> BT_cxx_call_null;<br>
> mutable OwningPtr<BugType> BT_cxx_call_undef;<br>
> mutable OwningPtr<BugType> BT_call_arg;<br>
> + mutable OwningPtr<BugType> BT_cxx_arg_undef;<br>
<br>
</div>The name is missing the word "delete". Maybe BT_cxx_delete_undef?<br>
<div class="im"><br>
<br>
> mutable OwningPtr<BugType> BT_msg_undef;<br>
> mutable OwningPtr<BugType> BT_objc_prop_undef;<br>
> mutable OwningPtr<BugType> BT_objc_subscript_undef;<br>
> @@ -44,6 +47,7 @@<br>
> public:<br>
><br>
> void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;<br>
> + void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;<br>
<br>
</div>Indentation is off.<br>
<div class="im"><br>
<br>
> void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const;<br>
> void checkPreCall(const CallEvent &Call, CheckerContext &C) const;<br>
><br>
> @@ -57,6 +61,9 @@<br>
> void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg,<br>
> ExplodedNode *N) const;<br>
><br>
> + void ReportBadDelete(const CXXDeleteExpr *DE, CheckerContext &C,<br>
> + BugType *BT, StringRef Desc, ExplodedNode *N) const;<br>
<br>
</div>Lowercase names for functions. (Yes, I see the one right below that doesn't follow this convention.) Also, since this is no longer handling different types of bad delete besides 'delete' and 'delete[]', maybe it makes sense to just fold it into the check method, like checkPreCall.<br>
<div class="im"><br>
<br>
> void HandleNilReceiver(CheckerContext &C,<br>
> ProgramStateRef state,<br>
> const ObjCMethodCall &msg) const;<br>
> @@ -250,6 +257,32 @@<br>
> C.addTransition(StNonNull);<br>
> }<br>
><br>
> +void CallAndMessageChecker::ReportBadDelete(const CXXDeleteExpr *DE,<br>
> + CheckerContext &C,<br>
> + BugType *BT,<br>
> + StringRef Desc,<br>
> + ExplodedNode *N) const {<br>
> +<br>
> + BugReport *R = new BugReport(*BT, Desc, N);<br>
> + bugreporter::trackNullOrUndefValue(N, DE, *R);<br>
> + C.emitReport(R);<br>
> +}<br>
> +<br>
> +void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE,<br>
> + CheckerContext &C) const {<br>
<br>
</div>Parameters not lined up?<br>
<div class="im"><br>
> +<br>
> + SVal RHS = C.getSVal(DE->getArgument());<br>
<br>
</div>Why 'RHS'? A 'delete' expression doesn't exactly have a "left-hand side" and "right-hand side".<br>
<div class="im"><br>
> + if (DE && RHS.isUndef()) {<br>
<br>
</div>You've already used DE unconditionally, so you can just drop that condition.<br>
<div class="im"><br>
> + ExplodedNode *N = C.generateSink();<br>
<br>
</div>It's possible that we've seen this state before, in which case 'N' will be null. Please check for null and return early if necessary<br>
<div class="im"><br>
> + if (!BT_cxx_arg_undef)<br>
> + BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));<br>
<br>
</div>This isn't really a memory error. I'd just go with BuiltinBug like the rest of the errors in this file.<br>
<div class="im"><br>
> + StringRef Desc = "Argument to delete is uninitialized";<br>
<br>
</div>Let's take the opportunity here to say "Argument to 'delete'" or "Argument to 'delete[]'" depending on how the delete was written. That kind of polish goes a long way.<br>
<br>
> + ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);<br>
<br>
Please follow the file's convention in putting spaces after commas. (Although again, it might make sense to just inline this function here.)<br>
<div><div class="h5"><br>
> + return;<br>
> + }<br>
> +}<br>
> +<br>
> +<br>
> void CallAndMessageChecker::checkPreCall(const CallEvent &Call,<br>
> CheckerContext &C) const {<br>
> ProgramStateRef State = C.getState();<br>
> Index: test/Analysis/NewDelete-checker-test.cpp<br>
> ===================================================================<br>
> --- test/Analysis/NewDelete-checker-test.cpp (revision 187717)<br>
> +++ test/Analysis/NewDelete-checker-test.cpp (working copy)<br>
> @@ -4,6 +4,7 @@<br>
><br>
> typedef __typeof__(sizeof(int)) size_t;<br>
> extern "C" void *malloc(size_t);<br>
> +extern "C" void free (void* ptr);<br>
> int *global;<br>
><br>
> //------------------<br>
> @@ -207,7 +208,26 @@<br>
> escapeVoidPtr(y);<br>
> } // no-warning<br>
><br>
> +//============== Test Uninitialized delete delete[]========================<br>
> +void testUninitDelete() {<br>
> + int *x;<br>
> + int * y = new int;<br>
> + delete x; // expected-warning{{Argument to delete is uninitialized}}<br>
> + delete y;<br>
> +}<br>
<br>
</div></div>Since you've declared the uninitialized delete to be a fatal error, we're never even getting to the second delete. That's actually something work checking: do a bad delete, then immediately do a null pointer dereference or something (*(volatile int *)0 = 1). You should get no warning on the second line.<br>
<br>
That does mean that you're going to want to test the good case first.<br>
<div class="im"><br>
<br>
> +void testUninitDeleteArray() {<br>
> + int *x;<br>
> + int * y = new int[5];<br>
> + delete[] x; // expected-warning{{Argument to delete is uninitialized}}<br>
> + delete[] y;<br>
> +}<br>
> +<br>
> +void testUninitFree() {<br>
> + int *x;<br>
> + free(x); // expected-warning{{Function call argument is an uninitialized value}}<br>
> +}<br>
> +<br>
<br>
</div>Thanks for working on this! It's almost ready to go in.<br>
<span class="HOEnZb"><font color="#888888"><br>
Jordan<br>
<br>
</font></span></blockquote></div><br></div>