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

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for adding handling of the symbolic cases! Some more comments inline.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:825
@@ -816,1 +824,3 @@
 
+ProgramStateRef CStringChecker::IsFirstBufInBound(CheckerContext &C,
+                                                  ProgramStateRef state,
----------------
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.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:840
@@ +839,3 @@
+  Optional<NonLoc> Length = LengthVal.getAs<NonLoc>();
+  if (!Length)
+    return state;
----------------
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?



================
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>()) {
+
----------------
Can you rewrite this if/else to avoid the indentation? (See http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).

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

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1115
@@ +1114,3 @@
+      // or have a symbolic offset.
+      if (SuperR) {
+        if (const ClusterBindings *C = B.lookup(SuperR)) {
----------------
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;

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

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

================
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}}
----------------
Should this test m->s3[j] as well?

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


http://reviews.llvm.org/D11832