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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks! I have felt that may I create a too complex symbolic value. Sorry for stealing your time.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2097-2098
 
+void CStringChecker::evalCharPtrCommon(CheckerContext &C,
+                                       const CallExpr *CE) const {
+  CurrentFunctionDescription = "char pointer returning function";
----------------
NoQ wrote:
> 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.
> I've no idea what it does
I wanted to represent the root string's VarDecl in the symbolic value so I could refer to it. I think it became too complex and printing out the root string's variable name does not worth that complexity. But here it is from `str30-c-explain.cpp`:
```
  char *slash;
  slash = strrchr(pathname, '/');

  clang_analyzer_dump(slash);
  // CHECK: &Element{pathname,conj_$1{long long, LC1, S{{[0-9]+}}, #1},char}

  clang_analyzer_explain(slash);
  // CHECK: pointer to element of type 'char' with index 'symbol of type 'long long' conjured at statement 'pathname'' of parameter 'pathname'
```

> 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.
At first the name of the string would be cool to print out and we need dataflow support to make it cool. Given that it is too complex I have reverted the modeling part. Also I wanted to create a simpler modeling with your modeling solution for my name-storing purposes, but I cannot.


================
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());
----------------
NoQ wrote:
> 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.
I wanted to force out the state split of an unknown indexing: null and non-null but may it was too explicit and useless, yes.


================
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);
----------------
NoQ wrote:
> This is guaranteed to return the string region that you already have as the value of that expression.
Whoops. The idea was that to handle string regions explicitly and may calculate the returning index as well.


================
Comment at: clang/test/Analysis/cert/str30-c-notes.cpp:29
+  if (slash) {
+    // expected-note at -1 {{'slash' is non-null}}
+    // expected-note at -2 {{Taking true branch}}
----------------
Charusso wrote:
> Needs to be an assumption.
Let us say it is non-null.


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

https://reviews.llvm.org/D71155