[analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[ayartsev]

Summary

Currently realloc(ptr, 0) is treated as free() which seems to be not correct. C standard (N1570) establishes equivalent behavior for malloc(0) and realloc(ptr, 0):
"7.22.3 Memory management functions calloc, malloc, realloc: 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."
The patch equalizes the processing of malloc(0) and realloc(ptr,0).
The patch also enables unix.Malloc checker to detect references to zero-allocated memory returned by realloc(ptr,0) ("Use of zero-allocated memory" warning).
Please review.

[ayartsev]

Ping.

[ayartsev]

Ping.

[zaks.anna]

C89 says: "If size is zero and ptr is not a null pointer, the object it points to is freed." I believe C89 and C99 disagree here.

I don't think we should add warnings for code that follows C89.

[joerg]

I don't believe we have to change anything here. The historic behavior of realloc(ptr, 0) is free(ptr) + returning NULL. That is also valid behavior under C11 where the behavior of malloc(0) is implemtation defined.

[ayartsev]

Updated the patch. Left an old behavior for C89. Please review.

In D9040#173371, @joerg wrote:

I don't believe we have to change anything here. The historic behavior of realloc(ptr, 0) is free(ptr) + returning NULL. That is also valid behavior under C11 where the behavior of malloc(0) is implemtation defined.

This behavior is not valid since C99. Under C99 and C11 the pure "If the size of the space requested is zero, the behavior is implementation-defined" (The C89 Draft) was restricted to "If the size of the space requested is zero, the ehavior 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." (C99 N1256 p.7.20.3; C11 N1570 p.7.22.3).

Another reason for tracking the return value from realloc(ptr, 0) is in ability to detect accesses to zero-allocated memory in realloc case which is currently missing. (http://reviews.llvm.org/D8273).

[joerg]

With malloc(0) returning NULL, it is exactly the same behavior as historically observed for realloc(foo, 0).

[ayartsev]

Yes, the behavior for realloc returning NULL remained unchanged (reallocPtrZero2() test handles this).

[ayartsev]

Ping.

[zaks.anna]

I am not convinced that this is the right approach.

Adding warnings about use of zero allocated memory is fine. However, introducing the new leak warnings is not as this might be the behavior people expect.

Keep in mind that we try to minimize the number of false positives in the analyzer; even if that means we are reducing the number of true positives.

[ayartsev]

Got it. Attached is an updated patch, please review.

[zaks.anna]

Here is a related enhancement request: https://llvm.org/bugs/show_bug.cgi?id=23695

[ayartsev]

The request is cancelled, unix.Malloc already handles both cases (usage after free and leak if realloc fails and reallocated memory is not freed.

[ayartsev]

Ping.

[zaks.anna]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

158

This struct does not contain any fields. What's its purpose?

512

Maybe you should use a set of SymbolRefs instead?

[ayartsev]

Please review!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

158

This struct is a flag that if attached indicates a zero-size reallocation. Improved class description in the updated patch.

512

This may produce false-positives as you explained me in D8273. Here is a modified example from D8273:

if (b)
  s= 10;
else
  s = 0;
int *p = malloc(8);
int *q = realloc(p, s);
if (b)
  *q = 1;

If the checker explores "realloc(p, s)" along the "s=0" path and add it to the set we'll get a false-positive along the "s=10" path later.

Included corresponding tests to the updated patch.

[ayartsev]

Ping

[zaks.anna]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

512

I do not think this is related to my question.

You add a map from a symbol to a "flag" here; not really a flag but the empty struct ReallocSizeZero. The only ways this is used is to set in the state that the symbol is zero realloced or query if the specific symbol is zero realloced. It seems that using the set of zero realloced symbols would be the right data structure here.

Why do we need the extra complexity of the map and the empty struct?

[ayartsev]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

512

Got it! Updated the patch.

[ayartsev]

Ping.

[ayartsev]

Updated the patch after r246978. Please review!

[ayartsev]

Ping.

[ayartsev]

Ping.

[zaks.anna]

See the suggestion for an improved comment. Otherwise, LGTM!

Thanks!
Anna.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

910

"Historically 'realloc(ptr, 0)' is treated as 'free(ptr)' and the returned value from 'realloc(ptr, 0)' is not tracked." -> "The value returned from 'realloc(ptr, 0)' is not tracked since historically 'realloc(ptr, 0)' is treated as 'free(ptr)' and the analyzer supports that model. However, we want to report uses of zero-allocated memory that get returned by realloc."

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

bool operator==(const ReallocPair &X) const {
     return ReallocatedSym == X.ReallocatedSym &&
            Kind == X.Kind;
   }
 };
 
 typedef std::pair<const ExplodedNode*, const MemRegion*> LeakInfo;
 
 class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PointerEscape,
                                      check::ConstPointerEscape,
                                      check::PreStmt<ReturnStmt>,
                                      check::PreCall,
                                      check::PostStmt<CallExpr>,
                                      check::PostStmt<CXXNewExpr>,
                                      check::PreStmt<CXXDeleteExpr>,
                                      check::PostStmt<BlockExpr>,
                                      check::PostObjCMessage,
public:
       }
     };
   };
 };
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, SymbolRef, RefState)
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
 // A map from the freed symbol to the symbol representing the return value of 
 // the free function.
 REGISTER_MAP_WITH_PROGRAMSTATE(FreeReturnValue, SymbolRef, SymbolRef)
 
 namespace {
 class StopTrackingCallback : public SymbolVisitor {
   ProgramStateRef state;
 public:
if (TrueState && !FalseState) {
     return TrueState->set<RegionState>(Sym,
                                        RefState::getAllocatedOfSizeZero(RS));
   }
 
   // Assume the value is non-zero going forward.
   assert(FalseState);
   return FalseState;
 }
 
 static QualType getDeepPointeeType(QualType T) {
   QualType Result = T, PointeeType = T->getPointeeType();
   while (!PointeeType.isNull()) {
     Result = PointeeType;
     PointeeType = PointeeType->getPointeeType();
   }
   return Result;
 }
ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
   ProgramStateRef StatePtrIsNull, StatePtrNotNull;
   std::tie(StatePtrIsNull, StatePtrNotNull) = State->assume(PtrEQ);
   ProgramStateRef StateSizeIsZero, StateSizeNotZero;
   std::tie(StateSizeIsZero, StateSizeNotZero) = State->assume(SizeZero);
   // We only assume exceptional states if they are definitely true; if the
   // state is under-constrained, assume regular realloc behavior.
   bool PrtIsNull = StatePtrIsNull && !StatePtrNotNull;
   bool SizeIsZero = StateSizeIsZero && !StateSizeNotZero;
+  bool IsC89 = !C.getLangOpts().C99 && !C.getLangOpts().CPlusPlus;
 
-  // If the ptr is NULL and the size is not 0, the call is equivalent to 
-  // malloc(size).
-  if ( PrtIsNull && !SizeIsZero) {
+  if (PrtIsNull) {
+    // The C89 Draft, p.4.10.3.4 : If ptr is a null pointer, the realloc
+    // function behaves like the malloc function for the specified size.
+    // The C89 Draft, p.4.10.3 : If the size of the space requested is zero,
+    // the behavior is implementation-defined.
+    if (IsC89 && SizeIsZero)
+      return nullptr;
+
+    // C99 (N1256) p.7.20.3 : 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.
+    //
+    // We keep tracking the returned pointer in case of a zero-allocation to
+    // check for zero-allocated memory access and to ensure that the memory is
+    // freed.
     ProgramStateRef stateMalloc = MallocMemAux(C, CE, CE->getArg(1),
-                                               UndefinedVal(), StatePtrIsNull);
+                                                 UndefinedVal(),
+                                                 StatePtrIsNull);
     return stateMalloc;
   }
 
-  if (PrtIsNull && SizeIsZero)
-    return nullptr;
-
   // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size).
-  assert(!PrtIsNull);
   SymbolRef FromPtr = arg0Val.getAsSymbol();
   SVal RetVal = State->getSVal(CE, LCtx);
   SymbolRef ToPtr = RetVal.getAsSymbol();
   if (!FromPtr || !ToPtr)
     return nullptr;
 
   bool ReleasedAllocated = false;
 
-  // If the size is 0, free the memory.
-  if (SizeIsZero)
+  if (IsC89 && SizeIsZero)
+    // The C89 Draft, p.4.10.3.4: If size is zero and ptr is not a null
+    // pointer, the object it points to is freed.
     if (ProgramStateRef stateFree = FreeMemAux(C, CE, StateSizeIsZero, 0,
                                                false, ReleasedAllocated)){
       // The semantics of the return value are:
       // If size was equal to 0, either NULL or a pointer suitable to be passed
       // to free() is returned. We just free the input pointer and do not add
       // any constrains on the output pointer.
       return stateFree;
     }

test/Analysis/malloc-annotations.c

 }
 
 void f2_realloc_0() {
   int *p = malloc(12);
   realloc(p,0);
   realloc(p,0); // expected-warning{{Attempt to free released memory}}
 }
 
-void f2_realloc_1() {
-  int *p = malloc(12);
-  int *q = realloc(p,0); // no-warning
-}
-
 // ownership attributes tests
 void naf1() {
   int *p = my_malloc3(12);
   return; // no-warning
 }
 
 void n2af1() {
   int *p = my_malloc2(12);

 void f6() {
   int *p = malloc(12);
   if (!p)
     return; // no-warning
   else
     free(p);
 }
 
-void f6_realloc() {
-  int *p = malloc(12);
-  if (!p)
-    return; // no-warning
-  else
-    realloc(p,0);
-}
-
-
 char *doit2();
 void pr6069() {
   char *buf = doit2();
   free(buf);
 }
 
 void pr6293() {
   free(0);

test/Analysis/malloc.c

 }
 
 void f2_realloc_0() {
   int *p = malloc(12);
   realloc(p,0);
   realloc(p,0); // expected-warning{{Attempt to free released memory}}
 }
 
-void f2_realloc_1() {
-  int *p = malloc(12);
-  int *q = realloc(p,0); // no-warning
-}
-
 void reallocNotNullPtr(unsigned sizeIn) {
   unsigned size = 12;
   char *p = (char*)malloc(size);
   if (p) {
     char *q = (char*)realloc(p, sizeIn);
     char x = *q; // expected-warning {{Potential leak of memory pointed to by 'q'}}
   }
 }
void reallocFails() {
   } else {
     free(r);
   }
 }
 
 void reallocSizeZero1() {
   char *p = malloc(12);
   char *r = realloc(p, 0);
-  if (!r) {
-    free(p); // expected-warning {{Attempt to free released memory}}
-  } else {
-    free(r);
-  }
-}
 
 void reallocSizeZero2() {
   char *p = malloc(12);
   char *r = realloc(p, 0);
-  if (!r) {
-    free(p); // expected-warning {{Attempt to free released memory}}
-  } else {
-    free(r);
-  }
-  free(p); // expected-warning {{Attempt to free released memory}}
-}
+} // expected-warning {{Potential leak of memory pointed to by}}
 
 void reallocSizeZero3() {
-  char *p = malloc(12);
-  char *r = realloc(p, 0);
-  free(r);
-}
-
-void reallocSizeZero4() {
   char *r = realloc(0, 0);
   free(r);
 }
 
-void reallocSizeZero5() {
+void reallocSizeZero4() {
   char *r = realloc(0, 0);
-}
+} // expected-warning {{Potential leak of memory pointed to by}}
 
 void reallocPtrZero1() {
   char *r = realloc(0, 12);
 } // expected-warning {{Potential leak of memory pointed to by 'r'}}
 
 void reallocPtrZero2() {
   char *r = realloc(0, 12);
   if (r)

 void CheckUseZeroAllocated6() {
   int *p = calloc(2, 0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
 void CheckUseZeroAllocated7() {
   int *p = realloc(0, 0);
-  *p = 1; //TODO: warn about use of zero-allocated memory
+  *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
 void CheckUseZeroAllocated8() {
   int *p = malloc(8);
   int *q = realloc(p, 0);
-  *q = 1; //TODO: warn about use of zero-allocated memory
+  *q = 1; // expected-warning {{Use of zero-allocated memory}}
   free(q);
 }
 
 void CheckUseZeroAllocated9() {
   int *p = realloc(0, 0);
   int *q = realloc(p, 0);
-  *q = 1; //TODO: warn about use of zero-allocated memory
+  *q = 1; // expected-warning {{Use of zero-allocated memory}}
   free(q);
 }
 
 void CheckUseZeroAllocatedPathNoWarn(_Bool b) {
   int s = 0;
   if (b)
     s= 10;
 

 void f6() {
   int *p = malloc(12);
   if (!p)
     return; // no-warning
   else
     free(p);
 }
 
-void f6_realloc() {
-  int *p = malloc(12);
-  if (!p)
-    return; // no-warning
-  else
-    realloc(p,0);
-}
-
-
 char *doit2();
 void pr6069() {
   char *buf = doit2();
   free(buf);
 }
 
 void pr6293() {
   free(0);

test/Analysis/realloc-C89.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify -std=c89 %s
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *realloc(void *ptr, size_t size);
+
+void reallocSizeZero1() {
+  char *p = malloc(12);
+  char *r = realloc(p, 0);
+  if (!r) {
+    free(p); // expected-warning {{Attempt to free released memory}}
+  } else {
+    free(r);
+  }
+}
+
+void reallocSizeZero2() {
+  char *p = malloc(12);
+  char *r = realloc(p, 0);
+  if (!r) {
+    free(p); // expected-warning {{Attempt to free released memory}}
+  } else {
+    free(r);
+  }
+  free(p); // expected-warning {{Attempt to free released memory}}
+}
+
+void reallocSizeZero3() {
+  char *p = malloc(12);
+  char *r = realloc(p, 0);
+  free(r); // no warning
+}
+
+void reallocSizeZero4() {
+  char *r = realloc(0, 0);
+  free(r); // no warning
+}
+
+void reallocSizeZero5() {
+  char *r = realloc(0, 0);
+} // no warning
+
+void CheckUseZeroAllocated1() {
+  int *p = realloc(0, 0);
+  *p = 1; // no warning
+  free(p);
+}
+
+void CheckUseZeroAllocated8() {
+  int *p = malloc(8);
+  int *q = realloc(p, 0);
+  *q = 1; // no warning
+  free(q);
+}
+
+void CheckUseZeroAllocated9() {
+  int *p = realloc(0, 0);
+  int *q = realloc(p, 0);
+  *q = 1; // no warning
+  free(q);
+}