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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
