[PATCH] [Review request][analyzer] Duplicate '0 size allocation' check from unix.API in unix.Malloc.

[Антон Ярцев]

Added the comment (inlined comment at MallocChecker.cpp, Line 259). OK to commit?


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:259
@@ +258,3 @@
+
+  /// \brief Perform a zero-allocation check.
+  ProgramStateRef ZeroAllocationCheck(CheckerContext &C, const CallExpr *CE,
----------------
Additional comment:

The request for zero bytes is suspicious.
C11 n1570, 7.22.3.1 "If the size of the space requested is zero, the behavior is implementation-defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object."

================
Comment at: test/Analysis/malloc-annotations.c:51
@@ -47,1 +50,3 @@
+  int *p = malloc(12);
+  realloc(p,0); // no-warning
 }
----------------
zaks.anna wrote:
> ayartsev wrote:
> > Quuxplusone wrote:
> > > FWIW, this maybe should give the same warning as `realloc(p,1);` on a line by itself: namely, because the returned pointer is never freed, this code has a memory leak on implementations where `realloc(p,0)` returns a non-null pointer.
> > > 
> > > If the returned pointer is captured (`q = realloc(p,0);`) and later freed (`free(q);`), there is no bug. It's not unsafe to use the return value of `realloc`; it's perfectly safe and well-defined. The only implementation-defined aspect of `realloc` is whether the returned pointer is null or not. Either way, *using* the returned pointer is fine — it's not like using a freed pointer, which is indeed undefined behavior.
> > //FWIW, this maybe should give the same warning as realloc(p,1); on a line by itself: namely, because the returned pointer is never freed, this code has a memory leak on implementations where realloc(p,0) returns a non-null pointer.//
> > Agree.
> > 
> > //It's not unsafe to use the return value of realloc; it's perfectly safe and well-defined. *using* the returned pointer is fine — it's not like using a freed pointer, which is indeed undefined behavior.//
> > It's not a safety check, just a warning that there may be an error in the code. The usage of a pointer returned from realloc(p,0) is suspicious, any bytes in the new object have indeterminate values also modifying the contents the returned memory is an error.
> > Zero size may be requested by reason of an error when the second parameter to realloc() is a variable.
> I don't think we should be warning when free is called on the returned value from realloc(p,0). We should try and stay away from reporting issues that would lead to known false positives. 
> 
> We could whitelist various ways the pointer could be dereferenced and warn on those or just not warn on realloc(p,0). (I think passing '0' to other allocation functions would not likely trigger false positives.)
> 
> Anton, what do you think?
> 
Currently we do not warn whether `free()` is called on the value returned from `realloc(p, 0)` or not. This is an old unmodified behavior.
The proposition is to warn if `free()` is //*not*// called on the returned value from `realloc(p, 0)` (other allocation functions as well) if the value is non-zero. May it is not a memory leak but I guess it may be some kind of a resource leak in case of a nonzero return because an information about a zero-allocated memory must be stored somewhere. Perhaps it is a good idea for a further enhancement.

//We could whitelist various ways the pointer could be dereferenced and warn on those or just not warn on realloc(p,0). (I think passing '0' to other allocation functions would not likely trigger false positives.)//
The usage of a pointer returned from a zero-allocation request is a matter of another potentional checker - ZeroAllocDereference which I plan to develop in the future. The goal of the current check is just to detect a zero-allocation. The current patch just moves '0 size allocation' check from unix.API to unix.Malloc with a small improvement - do not warn in the case when a pointer is not consumed. Perhaps as a next step it make sense to separate the check from the global unix.Malloc to a separate check - ZeroAlloc in order to make it switchable.

http://reviews.llvm.org/D6178

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/