[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);
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:184
+  if (IsFix) {
+    if (Optional<std::string> SizeStr = getSizeExprAsString(Call, CallC, C)) {
+      renameFunctionFix(UseSafeFunctions ? "gets_s" : "fgets", Call, *Report);
----------------
NoQ wrote:
> Also, which is probably more important, you will never be able to provide a fixit for the malloced memory case, because there may be multiple execution paths that reach the current point with different size expressions (in fact, not necessarily all of them are malloced).
> 
> Eg.:
> ```lang=c
> char *x = 0;
> char y[10];
> 
> if (coin()) {
>   x = malloc(20);
> } else {
>   x = y;
> }
> 
> gets(x);
> ```
> 
> If you suggest replacing `gets(x)` with `gets_s(x, 20)`, you'll still have a buffer overflow on the else-branch on which `x` points to an array of 10 bytes.
This checker going to evolve a lot, and all of the checked function calls have issues like that. I do not even think what else issues they have. I would like to cover the false alarm suppression when we are about to alarm. Is it would be okay? I really would like to see alarms first.

For example, I have seen stuff in the wild so that I can state out 8-param allocators and we need to rely on the checkers provide information about allocation.


================
Comment at: clang/test/Analysis/cert/str31-alloc.cpp:42
+  // expected-warning at -1 {{'gets' could write outside of 'buf3'}}
+  // CHECK-FIXES: if (gets_s(buf3 + 1, sizeof(buf3))) {}
+}
----------------
NoQ wrote:
> The fix is not correct. It should be `sizeof(buf3) - 1`, otherwise you still have a buffer overflow.
Good catch, thanks! I was really into the pretty-printing, we should not fix-it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69813