[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 ↗

(On Diff #26647)

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

524 ↗

(On Diff #26647)

Maybe you should use a set of SymbolRefs instead?

[ayartsev]

Please review!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

158 ↗

(On Diff #26647)

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

524 ↗

(On Diff #26647)

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

523 ↗

(On Diff #33134)

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

523 ↗

(On Diff #33134)

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

902 ↗

(On Diff #34583)

"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."

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

public:
       }
     };
   };
 };
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, SymbolRef, RefState)
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
+REGISTER_SET_WITH_PROGRAMSTATE(ReallocSizeZeroSymbols, SymbolRef)
 
 // 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 final : public SymbolVisitor {
   ProgramStateRef state;
ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C,
 
   if (TrueState && !FalseState) {
     SVal retVal = State->getSVal(E, C.getLocationContext());
     SymbolRef Sym = retVal.getAsLocSymbol();
     if (!Sym)
       return State;
 
     const RefState *RS = State->get<RegionState>(Sym);
-    if (!RS)
-      return State; // TODO: change to assert(RS); after realloc() will
-                    // guarantee have a RegionState attached.
-
-    if (!RS->isAllocated())
-      return State;
-
+    if (RS) {
+      if (RS->isAllocated())
         return TrueState->set<RegionState>(Sym,
                                           RefState::getAllocatedOfSizeZero(RS));
+      else
+        return State;
+    } else {
+      // Case of zero-size realloc. Historically 'realloc(ptr, 0)' is treated as
+      // 'free(ptr)' and the returned value from 'realloc(ptr, 0)' is not
+      // tracked. Add zero-reallocated Sym to the state to catch references
+      // to zero-allocated memory.
+      return TrueState->add<ReallocSizeZeroSymbols>(Sym);
+    }
   }
 
   // Assume the value is non-zero going forward.
   assert(FalseState);
   return FalseState;
 }
 
 static QualType getDeepPointeeType(QualType T) {
MallocChecker::getCheckIfTracked(CheckerContext &C,
                                  bool IsALeakCheck) const {
   return getCheckIfTracked(getAllocationFamily(C, AllocDeallocStmt),
                            IsALeakCheck);
 }
 
 Optional<MallocChecker::CheckKind>
 MallocChecker::getCheckIfTracked(CheckerContext &C, SymbolRef Sym,
                                  bool IsALeakCheck) const {
+  if (C.getState()->contains<ReallocSizeZeroSymbols>(Sym))
+    return CK_MallocChecker;
+
   const RefState *RS = C.getState()->get<RegionState>(Sym);
   assert(RS);
   return getCheckIfTracked(RS->getAllocationFamily(), IsALeakCheck);
 }
 
 bool MallocChecker::SummarizeValue(raw_ostream &os, SVal V) {
   if (Optional<nonloc::ConcreteInt> IntVal = V.getAs<nonloc::ConcreteInt>())
     os << "an integer (" << IntVal->getValue() << ")";
ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
   // malloc(size).
   if ( PrtIsNull && !SizeIsZero) {
     ProgramStateRef stateMalloc = MallocMemAux(C, CE, CE->getArg(1),
                                                UndefinedVal(), StatePtrIsNull);
     return stateMalloc;
   }
 
   if (PrtIsNull && SizeIsZero)
-    return nullptr;
+    return State;
 
   // 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 MallocChecker::checkUseAfterFree(SymbolRef Sym, CheckerContext &C,
   }
 
   return false;
 }
 
 void MallocChecker::checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
                                           const Stmt *S) const {
   assert(Sym);
-  const RefState *RS = C.getState()->get<RegionState>(Sym);
 
-  if (RS && RS->isAllocatedOfSizeZero())
+  if (const RefState *RS = C.getState()->get<RegionState>(Sym)) {
+    if (RS->isAllocatedOfSizeZero())
       ReportUseZeroAllocated(C, RS->getStmt()->getSourceRange(), Sym);
   }
+  else if (C.getState()->contains<ReallocSizeZeroSymbols>(Sym)) {
+    ReportUseZeroAllocated(C, S->getSourceRange(), Sym);
+  }
+}
 
 bool MallocChecker::checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const {
 
   if (isReleased(Sym, C)) {
     ReportDoubleDelete(C, Sym);
     return true;
   }
   return false;

cfe/trunk/test/Analysis/malloc.c

 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 CheckUseZeroAllocatedPathWarn(_Bool b) {
   char *p = malloc(s);
 
   if (b)
     *p = 1; // expected-warning {{Use of zero-allocated memory}}
 
   free(p);
 }
 
+void CheckUseZeroReallocatedPathNoWarn(_Bool b) {
+  int s = 0;
+  if (b)
+    s= 10;
+
+  char *p = malloc(8);
+  char *q = realloc(p, s);
+
+  if (b)
+    *q = 1; // no warning
+
+  free(q);
+}
+
+void CheckUseZeroReallocatedPathWarn(_Bool b) {
+  int s = 10;
+  if (b)
+    s= 0;
+
+  char *p = malloc(8);
+  char *q = realloc(p, s);
+
+  if (b)
+    *q = 1; // expected-warning {{Use of zero-allocated memory}}
+
+  free(q);
+}
+
 // This case tests that storing malloc'ed memory to a static variable which is
 // then returned is not leaked.  In the absence of known contracts for functions
 // or inter-procedural analysis, this is a conservative answer.
 int *f3() {
   static int *p = 0;
   p = malloc(12); 
   return p; // no-warning
 }