[PATCH] D152435: [analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D152435#4433147 <https://reviews.llvm.org/D152435#4433147>, @OikawaKirie wrote:

> BTW, what does the `Done` checkbox mean in the code comments?

"Done" means "Resolved" on GitHub - which is basically that some action was taken or the person who raised the issue withdrew.
In general, no revision/pull request should be merged with having open/unresolved discussions.
Usually, the patch author marks comments as "Done" and on the next update (by pressing the "Submit" button) all those threads will be closed automatically.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:286-287
+                      SVal V,
+                      std::function<bool(RegionAndSymbolInvalidationTraits &,
+                                         const MemRegion *)>);
 
----------------
OikawaKirie wrote:
> steakhal wrote:
> > I'd highly suggest making this a template taking the functor that way.
> > Given that we modify these functions, we should make them comply with the lower camel case naming convention.
> > And their variables with upper camel case.
> > I'd highly suggest making this a template taking the functor that way.
> 
> Any examples?
> Do you mean
> ```
> template <typename T>
> static ProgramStateRef
> InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
>                     SVal V, std::function<T> CallBack);
> ```
My problem is that `std::function` owns the callback, and really likely to allocate for doing so.
And in this context a more lightweight approach would be also good.
What I had in mind was:
```lang=C++
template <typename Callback>
static ProgramStateRef
InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
                    SVal V, Callback F);
...
bool asd = F(a, b);
```

But actually, `llvm::function_ref` would be probably even better, so use that instead.
Also, add some comments on what the callback's return type means etc.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1054
+    SVal SizeV, QualType SizeTy) {
+  return InvalidateBufferAux(
+      C, S, BufE, BufV,
----------------
OikawaKirie wrote:
> steakhal wrote:
> > Shouldn't we assert that `SizeV` is some known value or at least smaller than (or equal to) the extent of the buffer?
> The calls in `CStringChecker::evalCopyCommon` and `CStringChecker::evalStrcpyCommon` seem to have chances able to pass an unknown value. But I am not very sure about this.
> 
> If the `SizeV` is sure to be smaller or equal to the extent of the buffer, the `IsFirstBufInBound` check seems useless.
> Maybe I need to dig deeper into the callers.
Indeed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1075-1077
+      [](RegionAndSymbolInvalidationTraits &, const MemRegion *R) {
+        return MemRegion::FieldRegionKind == R->getKind();
+      });
----------------
OikawaKirie wrote:
> steakhal wrote:
> > The lambdas inline in the call look a bit messy. I would prefer declaring it and passing it as an argument separately.
> > This applies to the rest of the lambdas as well.
> > And their variables with upper camel case.
> 
> What about the variable names for the lambdas? Lower camel or upper camel?
> I cannot find the rules for named lambdas from the guidance <https://llvm.org/docs/CodingStandards.html>.
I think I asked the same question some time ago.
https://discourse.llvm.org/t/naming-lambdas-in-llvm-coding-standards/62689/3


================
Comment at: clang/test/Analysis/issue-55019.cpp:80-87
+  std::copy(p, p + 1, x.arr);
+
+  // FIXME: As we currently cannot know whether the copy overflows, the checker
+  // invalidates the entire `x` object. When the copy size through iterators
+  // can be correctly modeled, we can then update the verify direction from
+  // SymRegion to HeapSymRegion as this std::copy call never overflows and
+  // hence the pointer `x.ptr` shall not be invalidated.
----------------
OikawaKirie wrote:
> > So, you say that It should be `HeapSymRegion` instead because `x.arr` shouldn't be invalidated because the `std::copy` won't modify it. Right?
> 
> Yes, but just in this test case.
> 
> The size of `x.arr` is 4 and the copy length is 1, which means the call to `std::copy` here never overflows.
> Therefore, we should not invalidate the super region `x` and keep `x.ptr` still pointing to the original `HeapSymRegion` unchanged.
> However, the copy length computed through iterators is not modeled currently in `CStringChecker::evalStdCopyCommon`, where `InvalidateDestinationBufferAlwaysEscapeSuperRegion` is called.
> When it is modeled in the future, the function should call `InvalidateDestinationBufferBySize` instead to make the invalidation of the super region determined by the copy length to report potential leaks.
Makes sense. Thanks.


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

https://reviews.llvm.org/D152435