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

[Jordan Rose]

Looking good! Some inline comments on the patch itself, now.

On Aug 5, 2013, at 21:16 , Karthik Bhat <blitz.opensource at gmail.com> wrote:

> 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;

The name is missing the word "delete". Maybe BT_cxx_delete_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;

Indentation is off.


>    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;

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.


>    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 {

Parameters not lined up?

> +
> +  SVal RHS = C.getSVal(DE->getArgument());

Why 'RHS'? A 'delete' expression doesn't exactly have a "left-hand side" and "right-hand side".

> +  if (DE && RHS.isUndef()) {

You've already used DE unconditionally, so you can just drop that condition.

> +    ExplodedNode *N = C.generateSink();

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

> +    if (!BT_cxx_arg_undef)
> +      BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));

This isn't really a memory error. I'd just go with BuiltinBug like the rest of the errors in this file.

> +    StringRef Desc = "Argument to delete is uninitialized";

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.

> +    ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);

Please follow the file's convention in putting spaces after commas. (Although again, it might make sense to just inline this function here.)

> +    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;  
> +}

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.

That does mean that you're going to want to test the good case first.


> +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}}
> +}
> +

Thanks for working on this! It's almost ready to go in.

Jordan