[PATCH] D11832: [Patch] [Analyzer] false positive: Potential leak connected with memcpy (PR 22954)

[pierre gousseau via cfe-commits]

pgousseau added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:825
@@ -816,1 +824,3 @@
 
+ProgramStateRef CStringChecker::IsFirstBufInBound(CheckerContext &C,
+                                                  ProgramStateRef state,
----------------
dcoughlin wrote:
> It seems odd that you return a ProgramStateRef here but don't use it in the only caller of this function. Could you just return a boolean instead? Alternatively, if you really want to add the assumption that the buffer is valid to the post-state, then you should thread the state returned from the call to this function through the rest of CStringChecker::InvalidateBuffer.
Yes returning a bool would be better thanks, I did not meant for this function to add the assumption that the buffer is valid.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:840
@@ +839,3 @@
+  Optional<NonLoc> Length = LengthVal.getAs<NonLoc>();
+  if (!Length)
+    return state;
----------------
dcoughlin wrote:
> You have a lot of early returns of the form:
> 
>   if (!Foo)
>     return state;
> 
> that don't seem to be exercised in your tests. Can these actually trigger? If so, you should add tests for these cases. If not, maybe asserts/cast<T>()/castAs<T>() would be more appropriate.
> 
> Also: should these early returns return state? Or should they return null?
> 
> 
Yes most of those checks seems redundant and/or might need to return nullptr/false, will double check thanks.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:851
@@ +850,3 @@
+  SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType());
+  if (Optional<Loc> BufLoc = BufStart.getAs<Loc>()) {
+
----------------
dcoughlin wrote:
> Can you rewrite this if/else to avoid the indentation? (See http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).
Yes thanks for the link !

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:866
@@ +865,3 @@
+    assert(ER->getValueType() == C.getASTContext().CharTy &&
+           "CheckLocation should only be called with char* ElementRegions");
+
----------------
dcoughlin wrote:
> "CheckLocation" is copy pasta here.
Will fix !

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1115
@@ +1114,3 @@
+      // or have a symbolic offset.
+      if (SuperR) {
+        if (const ClusterBindings *C = B.lookup(SuperR)) {
----------------
dcoughlin wrote:
> This nesting is getting pretty deep. Can you invert some guards and change to goto/continue where appropriate.
> 
> For example:
>   if (!SuperR)
>     goto conjure_default;
> 
> and
> 
>   const ClusterBindings *C = B.lookup(SuperR)
>   if (!C)
>     continue;
Makes sense thanks.

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1125
@@ +1124,3 @@
+                 ((*ROffset >= LowerOffset && *ROffset < UpperOffset) ||
+                  (LowerOffset == UpperOffset && *ROffset == LowerOffset)))) {
+              B = B.removeBinding(I.getKey());
----------------
dcoughlin wrote:
> Please add a comment here to say that this is to handle arrays of 0 elements and arrays of 0-sized elements.
Will do !

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1132
@@ +1131,3 @@
+              if (R)
+                if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
+                  VisitBinding(V);
----------------
dcoughlin wrote:
> If you are not going to use the result, you can use isa<SymbolicRegion>(R) here instead of dyn_cast.
Will do !

================
Comment at: test/Analysis/pr22954.c:476
@@ +475,3 @@
+  clang_analyzer_eval(m->s3[3] == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(m->s3[i] == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l->s1[0] == 1); // expected-warning{{UNKNOWN}}
----------------
dcoughlin wrote:
> Should this test m->s3[j] as well?
Yes that is what I meant thanks !

================
Comment at: test/Analysis/pr22954.c:498
@@ +497,3 @@
+}
+
+
----------------
dcoughlin wrote:
> Can you also add a test where the size argument to memcpy is the result of sizeof()?
Will do !


http://reviews.llvm.org/D11832