[cfe-commits] [patch] Catch a few simple misuses of free()

[Ted Kremenek]

On Jun 4, 2010, at 10:48 PM, Jordy Rose wrote:

> Changed:
> - ignores unknown and undefined values.
> - more comments about what's being tested
> - messages that are hopefully more useful, which really is one of the best
> things about Clang. I'm a little worried that the messages are a bit long
> now.

I personally think the new diagnostics are awesome, but I'm thinking more about how they would be presented in scan-build output or an IDE rather than just the command line.  If you feel that they are too long, please feel free to shorten them.  I really love how they are very precise and clear.

Minor nit:

+  free((void*)&t10); // expected-warning {{Argument to free() is the address of the function t10, which is not memory allocated by malloc()}}

I'd include the name of the variable in quotes or some other way to distinguish it as a "name", e.g.:

  Argument to free() is the address of the function 't10', which is not memory allocated by malloc()

If you are thinking about shortening this, one possibility is:

  Value passed to free() was not returned from malloc() (value is the address of function 't10')

Static analyzer diagnostics actually support both a "short" and "long" version.  The short version, which gets emitted like standard compiler warnings, could be the text up to '(', while the long diagnostic (which gets displayed by scan-build) could be the whole thing.

Since you have done the work of creating the longer diagnostics, I'll let you make the call on whether or not (after seeing what they look like) you think they are too long.  I only suggest that you see how they look in scan-build's output first, as diagnostics that sometimes seem too long in terminal output are just fine in the web UI.

At any rate, I think the tone of the diagnostics are much better, even if you just leave it short.

> - accepts HeapSpaceRegion. I'm still not sure this is a good idea --
> things created by "new" shouldn't be free()d.


That's a valid concern, but I don't think you should optimize for that case yet.  Go ahead and add a big FIXME comment indicating your concern, as this is something that can be tackled as future work.  There are plenty of cases other than memory returned from 'new' that shouldn't be passed to free().  In order to check those well we will need to either extend the MemSpaceRegion interface to allowed "tagged" memory (e.g., memory returned by malloc()) or annotate symbolic regions as coming from malloc(), new, etc.  Real codebases can contain custom allocators, and the memory returned from those shouldn't be passed to free() either.

Nice work!