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/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
