[PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
+                                             const CXXNewExpr *NE,
----------------
NoQ wrote:
> dkrupp wrote:
> > xazax.hun wrote:
> > > zaks.anna wrote:
> > > > I am not sure this code belongs to the malloc checker since it only supports the array bounds checker. Is there a reason it's not part of that checker?
> > > I think it is part of the malloc checker because it already does something very very similar to malloc, see the MallocMemAux function. So in fact, for the array bounds checker to work properly, the malloc checker should be turned on.
> > Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and CStringChecker checkers currently. New expression in case of simple allocations (0 allocation) was already handled in Malloc checker , that's why I implemented it there. But I agree it feels odd that one has to switch on unix.Malloc checker to get the size of new allocated heap regions. Should I move this to ArrayBoundChecker or ArrayBoundCheckerV2?
> 1. Well, in my dreams, this code should be in the silent operator-new modelling checker, which would be separate from the stateless operator-new bug-finding checker. Then all the checkers that rely on extent would automatically load the modelling checker as a dependency.
> 
> 2. Yeah, i think many checkers may consider extents, not just array bound; other checkers may appear in the future. This info is very useful, which is why we have the whole symbol class just for that (however, checker communication through constraints on this symbol class wasn't IMHO a good idea, because it's harder for the constraint manager to deal with symbols and constraints rather than with concrete values).
> 
> //Just wanted to speak out, thoughts below might perhaps be more on-topic//
> 
> 3. The MallocChecker is not just unix.Malloc, but also cplusplus.NewDelete, etc., which makes a lot more sense to leave it here.
> 
> 4. Consider another part of MallocChecker's job - modeling the memory spaces for symbolic regions (heap vs. unknown). For malloc(), this is done in the checker. For operator-new, it is done in the ExprEngine(!), because this is part of the language rather than a library function. Perhaps ExprEngine would be the proper place for that code as well.
> Well, in my dreams, this code should be in the silent operator-new modelling checker, which would be 
> separate from the stateless operator-new bug-finding checker. Then all the checkers that rely on extent 
> would automatically load the modelling checker as a dependency.

I agree. This sounds like the best approach! (Though it's not a must have to get the patch in.)

> Consider another part of MallocChecker's job - modeling the memory spaces for symbolic regions (heap vs. 
> unknown). For malloc(), this is done in the checker. For operator-new, it is done in the ExprEngine(!), because 
> this is part of the language rather than a library function. Perhaps ExprEngine would be the proper place for 
> that code as well.

Interesting point. Can you clarify the last sentence?

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1020
@@ +1019,3 @@
+  } else {
+    Size = svalBuilder.makeIntVal(1, true);
+    Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
----------------
Please, be more explicit that this is not a size of allocation (as in not 1 byte)? Maybe call this ElementCount or something like that.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1021
@@ +1020,3 @@
+    Size = svalBuilder.makeIntVal(1, true);
+    Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
+  }
----------------
A bit of code repetition from above. Please, add comments explaining why we need subregion in one case and super region in the other.

Are there test cases for this branch?

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1049
@@ +1048,3 @@
+                               CharUnits Scaling, SValBuilder &Sb) {
+  return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
+                        Sb.makeArrayIndex(Scaling.getQuantity()),
----------------
This should probably be inline/have different name since it talks about ArrayIndexType and is not reused elsewhere.

================
Comment at: test/Analysis/out-of-bounds.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-array-bounds -analyze -analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+
----------------
Let's use a more specific file name.


https://reviews.llvm.org/D24307