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

[Anna Zaks]

Hi Anton,

Looks like the right direction. See a few comments throughout the code.

How do you plan on staging this? The current patch cannot be committed as is for the following 2 reasons:
 - Some warnings will be duplicated since we warn here and in the unix.api check. 
 - We should not warn on realloc(p,0).

The proposed treatment of realloc seems fine to me. Maybe you could just start with skipping the warning on realloc to stage the commits. What do you think?

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:222
@@ -220,4 +221,3 @@
   mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc,
-                         *II_valloc, *II_reallocf, *II_strndup, *II_strdup,
-                         *II_kmalloc, *II_if_nameindex, *II_if_freenameindex;
+  mutable std::unique_ptr<BugType> BT_MallocZero[CK_NumCheckKinds];
+  mutable IdentifierInfo *II_alloca, *II_alloca_builtin, *II_malloc, *II_free,
----------------
Ho about BT_MallocZero -> BT_ZerroAllocation ?

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:259
@@ +258,3 @@
+
+  ProgramStateRef BasicAllocationCheck(CheckerContext &C, const CallExpr *CE,
+                                       const unsigned NumArgs, 
----------------
Please, add oxygen comment for this and "CheckCallocZero".

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:261
@@ +260,3 @@
+                                       const unsigned NumArgs, 
+                                       const unsigned SizeArg,
+                                       ProgramStateRef State) const;
----------------
I'd rename "SizeArg" to make it more clear it's the allocation size; for example, something like "AllocationSizeArg".

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:817
@@ -792,1 +816,3 @@
 
+//===----------------------------------------------------------------------===//
+// "calloc", "malloc", "realloc", "reallocf", "alloca" and "valloc"
----------------
This comment is probably copied from the unix.api checker, where it tells which APIs are being processed below. It feels out of place here.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:823
@@ +822,3 @@
+// Returns true if we try to do a zero byte allocation, false otherwise.
+// Fills in TrueState and TalseState.
+static bool IsZeroByteAllocation(ProgramStateRef State,
----------------
TalseState -> FalseState

Are the True and False states backwards? I would expect TrueState to mean zero allocation; but by looking at the return it seems that it's the opposite..

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:834
@@ +833,3 @@
+
+// Does a basic check for 0-sized allocations suitable for most of the below
+// functions (modulo "calloc")
----------------
Does -> Performs

"most of the bellow"? where below?

Missing "."

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:845
@@ +844,3 @@
+  // Sanity check for the correct number of arguments
+  if (CE->getNumArgs() != NumArgs)
+    return nullptr;
----------------
Does it make sense to pass an extra argument just for sanity check?

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:866
@@ +865,3 @@
+
+ProgramStateRef MallocChecker::CheckCallocZero(CheckerContext &C,
+                                               const CallExpr *CE,
----------------
Why is CheckCallocZero different from BasicAllocationCheck? (I feel like I am missing something..)

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1548
@@ +1547,3 @@
+void MallocChecker::ReportZeroByteAllocation(CheckerContext &C,
+                                             ProgramStateRef FalseState,
+                                             const Expr *Arg,
----------------
Please, use more descriptive state name.

================
Comment at: test/Analysis/malloc.c:957
@@ -951,1 +956,3 @@
 // ----------------------------------------------------------------------------
+// Test zero-sized allocations.
+void testMallocZero() {
----------------
Could you add a couple of more interesting zero allocation tests (where we are not dealing with zero constant).

Theoretically, the inlined defensive checks could be a problem here, right?
(This is where the value is assumed to be zero by an inlined function, but known not be be zero in a caller.)

http://reviews.llvm.org/D6178