[PATCH] D71155: [analyzer] CERT: STR30-C

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Let's separate `CStringChecker` improvements into a separate patch and have a separate set of tests for it.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2097-2098
 
+void CStringChecker::evalCharPtrCommon(CheckerContext &C,
+                                       const CallExpr *CE) const {
+  CurrentFunctionDescription = "char pointer returning function";
----------------
Ok, so this whole thing is trying to evaluate `strchr`-like functions, right? I've no idea what it does but the problem is much more trivial that what you're trying to do here.

Branch 1:
1. Conjure the offset.
2. Add it to the original pointer (using `evalBinOp`).
3. Bind the result.
Branch 2:
1. Bind null.

And you probably should drop branch 2 completely because usually there's no indication that the character may in fact be missing in the string, and i don't want more null dereference false alarms. So it's just branch 1.

So the whole function should be 3 lines of code (maybe with a couple line breaks).

Well, maybe you should also handle the case where the original pointer is null (not sure if it's an immediate UB or not).

This could be improved by actually taking into account the contents of the string, but you don't seem to be trying to do this here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2101-2103
+  SValBuilder &SVB = C.getSValBuilder();
+  ProgramStateRef State, StateNull;
+  std::tie(StateNull, State) = C.getState()->assume(SVB.makeNull());
----------------
So, like, `StateNull` is the state in which it is assumed that `0` is non-zero and `State` is the state in which it is assumed that `0` is zero?

I mean, apart from the naming flip-flop - i can tell you in advance that `0` is zero, it's not a matter of assumptions.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127-2128
+  if (const auto *SL = dyn_cast<StringLiteral>(SrcExpr->IgnoreImpCasts())) {
+    const StringRegion *ResultMR =
+        C.getStoreManager().getRegionManager().getStringRegion(SL);
+    SVal ResultV = loc::MemRegionVal(ResultMR);
----------------
This is guaranteed to return the string region that you already have as the value of that expression.


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

https://reviews.llvm.org/D71155