[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
----------------
NoQ wrote:
> Naming: I believe we should keep using the word "Extent". There's no need to introduce a new term for the existing concept; it makes it harder to explain on the mailing list :) Let's make a follow-up patch to change the naming back (so that not to screw the review).
Since then I have changed my mind when I have read about Environment and Store in a book. Sure, next up.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+                               const CXXNewExpr *NE,
+                               const LocationContext *LCtx, SValBuilder &SVB);
----------------
NoQ wrote:
> This function is probably going to be used exactly once in the whole code. There's no need to turn it into a public API.
It is being used 3 times already, so I believe it is a cool API.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+          .Case("clang_analyzer_region", &ExprInspectionChecker::analyzerRegion)
+          .Case("clang_analyzer_size", &ExprInspectionChecker::analyzerDumpSize)
+          .Case("clang_analyzer_elementCount",
----------------
NoQ wrote:
> `clang_analyzer_dump_extent()`? Or just `clang_analyzer_dump(clang_analyzer_getExtent())`.
I like the shorter version, but of course I have seen the longer version.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);
----------------
NoQ wrote:
> So, how is it different from the existing `clang_analyzer_dump()`?
I wanted to make it for the derived regions, but then I have realized I am lazy to dig into the buggier parts of the Analyzer. Removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+                    : CE->getArg(0)->IgnoreParenImpCasts()->getType();
+
----------------
NoQ wrote:
> How is this better than `getValueType()`? Are you sure you're not getting a pointer type instead in the `!TVR` case? I.e., can you test this on a non-heap `SymRegion`?
> How is this better than getValueType()?
Consistency. We get the static ~~size~~ extent by getting the desugared type which most likely just an extra overhead.

> Are you sure you're not getting a pointer type instead in the !TVR case? I.e., can you test this on a non-heap SymRegion?
The issue was the `var_region_simple_ptr` test: we need to use regex for checking after the `}}` token where `/ 4` is the correct result. I did not really get why the test pass.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+    if (CI->getValue().isUnsigned())
+      Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+
----------------
NoQ wrote:
> Charusso wrote:
> > That one is interesting. Some of the checkers / SValBuilder(?) generate unsigned integers which should not happen, I believe. May we need a FIXME and an assertion about signedness. What do you think?
> `SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept `size_t` as a parameter. Therefore everything needs to be //unsigned// and we need to assert this.
> 
> That said, array indexes are //signed// (as per implementation of `ElementRegion`).
It was a premature optimization for consistency. I will leave the signedness as is, FIXME added.


================
Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+        tmp2[x] = am; // expected-warning \
+        {{Access out-of-bound array element (buffer overflow)}}
       }
----------------
NoQ wrote:
> Charusso wrote:
> > That is the single regression which I do not get.
> Well, please debug. Like, look at the full report, dump egraph, see what changed. Try to `creduce` the example further under the condition "behavior changed with the patch", maybe that'll clear something up (though if it's a true positive after creduce it doesn't guarantee that it's a true positive before creduce).
Given that I have no access for `rdar` I did my hand-reduce and the solution is the following:
```lang=c
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 \
// RUN:  -analyzer-checker=core,alpha.security.ArrayBound \
// RUN:  -verify %s

static void RDar8424269_B(int unknown) {
  unsigned char tmp2t[1][unknown];
  tmp2t[1][13] = 0; // expected-warning \
    {{Access out-of-bound array element (buffer overflow)}}
}
```

Looking at the master branch `core.VLASize` created the constraint `extent_$1{tmp2} {[0, 0]}` without any state splits. The new behavior does not create random constraints so that an alpha checker could detect we have no information about the unknown ~~size~~ extent.

Given that the alpha checker cannot catch the following: `char foo[bar]; foo[13] = 0;` is assumed that the problem is the VLASizeChecker's modeling which has a FIXME: `Handle multi-dimensional VLAs.` but does not handle this case.

An easy and appropriate fix to detect the multi-dimensional array. Here is the way to obtain the inner dimensions: http://clang-developers.42468.n3.nabble.com/Multi-dimensional-arrays-td4028727.html

So that the final solution is:
```lang=diff
   // FIXME: Handle multi-dimensional VLAs.
+  if (VLA->getElementType()->getAsArrayTypeUnsafe())
+    return;
```

I hope with that I have fixed even more Bugzilla issues. May it would be interesting to see why the ~~size~~ extent of the array got a constraint of being `0`, but I leave it as an exercise for the reader.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726