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

[Ella Ma via Phabricator via cfe-commits]

OikawaKirie added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1054
+    SVal SizeV, QualType SizeTy) {
+  return InvalidateBufferAux(
+      C, S, BufE, BufV,
----------------
steakhal wrote:
> 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.
Function `CStringChecker::CheckBufferAccess` is responsible to check whether the size overflows.
However, when `Filter.CheckCStringOutOfBounds` is set to false, the function will skip the overflow checking.
This makes that we cannot assert SizeV is always inbound, and the check in `IsFirstBufInBound` seems to be necessary.
(`evalMemset` -> `CheckBufferAccess`(skip overflow checking) -> `memsetAux` -> `InvalidateDestinationBufferBySize` with function call of `memset(x.arr, 'a', 42)`)

Besides, if the size argument is originally an unknown value expression (e.g. `(unsigned char) 1.0`), the unknown value can be passed to this function.
(`CStringChecker::evalCopyCommon` -> `invalidateDestinationBufferBySize`, with function call of `memcpy(x.arr, "hi", (unsigned char) 1.0)`)

Hence, we cannot make such an assertion here. : (


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

https://reviews.llvm.org/D152435