[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

-24

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

-27

Maybe you should use a set of SymbolRefs instead?

[ayartsev]

Please review!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

-24

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

-27

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

-27

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

-27

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

-21

"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

 //=== MallocChecker.cpp - A malloc/free checker -------------------*- C++ -*--//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
if (!Arg1)
     return nullptr;
 
   // Get the value of the size argument.
   SVal Arg1ValG = State->getSVal(Arg1, LCtx);
   if (!Arg1ValG.getAs<DefinedOrUnknownSVal>())
     return nullptr;
   DefinedOrUnknownSVal Arg1Val = Arg1ValG.castAs<DefinedOrUnknownSVal>();
 
-  // Compare the size argument to 0.
-  DefinedOrUnknownSVal SizeZero =
-    svalBuilder.evalEQ(State, Arg1Val,
-                       svalBuilder.makeIntValWithPtrWidth(0, false));
-
   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;
-
-  // If the ptr is NULL and the size is not 0, the call is equivalent to 
-  // malloc(size).
-  if ( PrtIsNull && !SizeIsZero) {
+  // If the ptr is NULL the call is equivalent to malloc(size).
+  if (StatePtrIsNull && !StatePtrNotNull) {
     ProgramStateRef stateMalloc = MallocMemAux(C, CE, CE->getArg(1),
                                                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 (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;
-    }
-
   // Default behavior.
   if (ProgramStateRef stateFree =
         FreeMemAux(C, CE, State, 0, false, ReleasedAllocated)) {
 
     ProgramStateRef stateRealloc = MallocMemAux(C, CE, CE->getArg(1),
                                                 UnknownVal(), stateFree);
     if (!stateRealloc)
       return nullptr;

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);