[PATCH] D69813: [analyzer][WIP] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:124
+  if (const SymbolicRegion *SR = DestMR->getSymbolicBase())
+    if (const Expr *SizeExpr = getDynamicSizeExpr(C.getState(), SR))
+      return exprToStr(SizeExpr, C);
----------------
Charusso wrote:
> NoQ wrote:
> > Again, you will have to highlight the allocation site with a note. Therefore you will have to write a bug visitor that traverses the size expression at some point (or, equivalently, a note tag when the size expression is evaluated). Therefore you don't need to store the expression in the program state.
> Yes, you have pointed out the necessary visitor, but it needs more thinking.
> 
> I have a memory region which could be any kind of "memory block region" therefore I have no idea where is the size expression. We are supporting ~20 different allocations, which is nothing compared to the wild with the not so uncommon 5+ parameter allocators. Therefore I still do not want to reverse engineer a small MallocChecker + ExprEngine + BuiltinFunctionChecker inside my checker. They provide the necessary `DynamicSizeInfo` easily, which could be used in at least 4 checkers at the moment (which I have pointed out earlier in D68725).
> 
> If I have the size expression in the dynamic size map, and I can clearly point out the destination buffer, it is a lot more simplified to traverse the graph where the buffer and its size comes from.
Well, you really do not want to store `SizeExpr` of `malloc(SizeExpr)` and you are right I will have to traverse from it to see whether the `SizeExpr` is ambiguous or not, where it comes from.

I want to rely on the `trackExpressionValue()` as the `SizeExpr` is available by `getDynamicSizeExpr()`, so it is one or two lines of code.

Would you create your own switch-case to see where is the size expression goes in the allocation and use `trackExpressionValue()` on it? So that you do not store information in the global state which results in better run-time / less memory.

At first I really wanted to model `malloc()` and `realloc()` and stuff, then I realized the `MallocChecker` provides every information I need. Would it be a better idea to create my own tiny `MallocChecker` inside my checker which does nothing but marks the size expression being interesting with `NoteTags`?

Also I am thinking of a switch-case on the `DefinedOrUnknownSVal Size` which somewhere has an expression inside it which I could `trackExpressionValue()` on.

Basically we are missing the rules what to use and I have picked the easiest solution. Could you share please which would be the right direction for such a simple task?


Repository:
  rC Clang

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

https://reviews.llvm.org/D69813