[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I've a feeling we need to roll this back a little bit.

The `memset()` function does the following:

1. Accesses pointers in range R = [first argument, first argument + third argument] and crashes when accessing an invalid pointer.
2. Writes second argument to all bytes in range R.
3. Returns its first argument.

Assuming R is an empty set, steps 1 and 2 are skipped.

We handle step 1 through `checkNonNull` and `checkBufferAccess`. These easy-to-use functions are already available in the checker, just pass the arguments there directly.

For step 2, we decided to skip most of the step, instead invalidating the whole //base region// around R. There's a separate task to come up with a better behavior here, but we decided to do it later, probably in the next patch. So for now, the relationship between the size of the buffer and the third argument are not relevant and should not be considered - this is an idea for the future.

Finally, step 3 can be done by binding the call expression to the symbolic value of the first argument through `BindExpr`. You are already doing this in the zero-size case, but it should be similar in all cases. There is no need to construct a new symbol here - just use the existing `SVal`.



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2034
+  // If the size can be nonzero, we have to check the other arguments.
+  if (stateNonZeroSize) {
+    state = stateNonZeroSize;
----------------
danielmarjamaki wrote:
> use early return:
> 
>   if (!stateNonZeroSize)
>     return;
In fact this early return is unnecessary. Of the two states returned by `assume()`, at least one is always non-null (we have an assertion for that). However, the since the situation `(StateZeroSize && !StateNonZeroSize)` was checked above, it follows from `(!StateNonZeroSize)` that `StateZeroSize` is also null, which contradicts the above.

So we can assert here that `StateNonZeroSize` is not null.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2045-2049
+  SVal RetVal = SB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount());
+  const SymbolicRegion *SR =
+      dyn_cast_or_null<SymbolicRegion>(RetVal.getAsRegion());
+  if (!SR)
+    return;
----------------
Here you construct a new symbol that represents the pointer returned. However, we return our first argument, which is already denoted by symbolic value `MemVal`, so we don't need a new symbol here - we'd return `MemVal` directly, and work on it directly during invalidation.

Also, note that it's not necessarily on the heap.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868