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

Authored by ayartsev on Apr 15 2015, 2:33 PM.

Details

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.

Diff Detail

Repository
rL LLVM
ayartsev retitled this revision from to [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0)..Apr 15 2015, 2:33 PM
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: zaks.anna.
ayartsev added subscribers: jordan_rose, krememek, Unknown Object (MLST).

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 added a subscriber: joerg.May 15 2015, 1:53 AM

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 this revision to Diff 26046.May 19 2015, 4:31 AM

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 added a comment.May 19 2015, 4:35 AM

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

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

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 updated this revision to Diff 26647.May 27 2015, 5:03 PM

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

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

zaks.anna added inline comments.Jun 17 2015, 10:23 PM
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 updated this revision to Diff 33134.Aug 25 2015, 3:12 PM

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.

zaks.anna added inline comments.Sep 1 2015, 2:03 PM
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 updated this revision to Diff 33740.Sep 1 2015, 3:01 PM
ayartsev added inline comments.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
523 ↗(On Diff #33134)

Got it! Updated the patch.

ayartsev updated this revision to Diff 34583.Sep 11 2015, 1:49 PM

Updated the patch after r246978. Please review!

zaks.anna accepted this revision.Sep 21 2015, 6:45 PM

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

This revision is now accepted and ready to land.Sep 21 2015, 6:45 PM
This revision was automatically updated to reflect the committed changes.