[PATCH] D27753: [analyzer] alpha.security.DirtyScalar Checker

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Zoltan. Sorry, I'm a bit busy now. Here are my thoughts about the design.

1. I think we should not add new warnings to GenericTaintChecker. Instead, it is better to move warnings out of it.  After that it will become just a plugin used by other checkers. Such split will make it cleaner because we avoid mixing taint propagating logic and checks. It is not an item for this patch, of course. This new checker follows my preferences in this part.
2. For me, taint-related check for array index in this checker and in ArrayBoundCheckerV2 look almost the same if the offset is known (with just a single difference). However, in case of tainted check we can be less conservative so approach used in this checker when a warning is emitted even if base region or offset are unknown looks better for me. I think we should remove tainted check from ArrayBoundV2 and leave it in this new checker. But this causes a situation where out-of-bound check is not done in checker intended for this.  A possible solution here is to move array-related logic of DirtyScalar to a separate checker that is enabled when ArrayBound or DirtyScalar are enabled. Zoltán, could you confirm that your checker emits same warnings on ArrayBoundChecker test cases with taint?

Anna, what's your opinion? Did I miss something?



================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:184
+    Ty = Ctx.IntTy;
+  if (!Ty->isIntegerType() || Ctx.getIntWidth(Ty) <= TooNarrowForBoundCheck)
+    return false;
----------------
Does the second check means that we exclude boolean and char values? I cannot find any reason to do it for chars.


https://reviews.llvm.org/D27753