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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks correct! Thanks for taking this up.

One of the possible improvements for future work here would be to actually bind the second argument value to the buffer instead of just invalidating it. Like, after `memset(buf, 0, sizeof(buf))` the analyzer should know that all values in the `buf` array are `0`. In the analyzer we have the notion of *default bindings* to handle that (see documentation in docs/analyzer/RegionStore.txt for more details). However, they would only work when the whole region is `memset`ed, not when we're wiping a smaller portion of the region. But the special case of `memset`ing the whole region is important enough to be worth handling separately anyway. In order to detect this special case, you can find the region's *extent* and see if it's exactly equal to the size argument (ideally, handle comparison with the `assume` method). This would work not only for arrays, but also for regions allocated via `malloc` or C++ operator `new`. There is an attempt to do a similar thing for `strdup` in https://reviews.llvm.org/D20863 (mixed with more complicated stuff).

Since we're in `CStringChecker`, we may also want to see how `memset` affects our model of string length. If we `memset` to zero everything from the beginning of the region, the length of the resulting C string is 0; if not to zero, then the length of the resulting C string is at least the size argument; if not from the beginning, then we've no idea, unless we knew the length before and it was less then the offset from which we started `memset`ing.



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2025
+
+  // If the size is zero, there won't be any actual memory access, so           
+  // just bind the return value to the destination buffer and return.
----------------
Trailing spaces here.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2037
+
+    // Ensure the memory area pointed to by s is not null. 
+    // If it is NULL there will be a NULL pointer dereference.
----------------
One more trailing space.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2051-2052
+
+    // Invalidate the memory area pointed to by s (regular invalidation without 
+    // pointer-escaping the address of the top-level region). 
+    state = InvalidateBuffer(C, state, S, C.getSVal(S),
----------------
And a few more trailing spaces :) I usually catch those with a colored `git diff` and highlight them in my editor as well, and also have a hotkey to clang-format current line, which is probably the most handy.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868