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

[Artem Dergachev via cfe-commits]

NoQ added a comment.

Thanks for the patch! Not sure why, but i always have warm feelings for the `MallocChecker` and wish it to improve.

Added minor style comments, joined the "where to put the code" debate.


================
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:69
@@ -68,2 +68,3 @@
 
 static SVal computeExtentBegin(SValBuilder &svalBuilder,
+                               const MemRegion *region, ProgramStateRef state) {
----------------
For ease of use, you no longer need to pass `svalBuilder` here - you can take it from the `state->getStateManager()`.

================
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:81
@@ +80,3 @@
+      // If we known the symbolic region extent size
+      //(e.g. allocated by malloc or new)
+      // we can assume that the region starts at 0.
----------------
Whitespace slightly off.

================
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:83
@@ -78,1 +82,3 @@
+      // we can assume that the region starts at 0.
+      if (!state->isNull(extentVal).isConstrained()) {
         return UnknownVal();
----------------
Perhaps you could consider the memory space of the `region`, it would look a bit less hacky to me.

In my dreams, i wish heap regions were no longer symbolic regions, and this hack would go away then.

Also, i recall there is a bug in `isNull()`: in the `ConstraintManager` class (this time i actually mean //the abstract base class// of `RangeConstraintManager`) this function boils down to `assume()`, but in `RangeConstraintManager` it is overridden to do a direct lookup into the constraint map; which means that in fact this function does not simplify symbolic expressions before answering. This code is probably unaffected because extents are always either concrete or atomic symbols, but i think i'd make a patch for that.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995
@@ -983,2 +994,3 @@
                                                            : AF_CXXNew);
+  State=addExtentSize(C,NE,State);
   State = ProcessZeroAllocation(C, NE, 0, State);
----------------
This code could use a bit more spaces around "=" and ",".

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
+                                             const CXXNewExpr *NE,
----------------
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.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1010
@@ +1009,3 @@
+  SVal Size;
+  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+  const MemRegion *Region;
----------------
`C.getLocationContext()` is a bit shorter.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1017
@@ +1016,3 @@
+                 .getAsRegion()
+                 ->getAs<SubRegion>()
+                 ->getSuperRegion();
----------------
You can `castAs<>` here. Generally, there's no point in using `getAs<>` unless you're going to check later.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1026
@@ +1025,3 @@
+  // Set the region's extent equal to the Size parameter.
+  const SymbolicRegion *R = dyn_cast<SymbolicRegion>(Region);
+  if (!R)
----------------
I think you're not using the fact that R is a `SymbolicRegion`. You could have as well written `if (!isa<SymbolicRegion>(Region)) return`.

This would also make variable names a bit more confusing.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1048
@@ +1047,3 @@
+SVal MallocChecker::scaleValue(ProgramStateRef State, NonLoc BaseVal,
+                               CharUnits Scaling, SValBuilder &Sb) {
+  return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
----------------
Here also you can extract the `SValBuilder` from the state.

================
Comment at: test/Analysis/out-of-bounds.cpp:143
@@ +142,3 @@
+void test_diff_types(int x) {
+  int *buf = new int[10]; //40 Bytes allocated
+  char *cptr = (char *)buf;
----------------
I think you should specify the target triple in the test run-line. Otherwise some buildbot with weird `int` size might fail (as the triple defaults to the current machine's triple).


https://reviews.llvm.org/D24307