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

[Jordan Rose]

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