[PATCH] D9924: Ignore report when the argument to malloc is assigned known value

[Aditya Kumar via cfe-commits]

hiraditya added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp:182
@@ +181,3 @@
+      if (rhs->isEvaluatable(Context))
+        eraseAssign = true;
+      // Erase if the multiplicand was assigned a value,
----------------
zaks.anna wrote:
> In this case, the size will be a multiplication os two constants, which we will assume cannot be exploitable, so seems legitimate. (Maybe spell this out in the comment?)
Done.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp:186
@@ +185,3 @@
+      // is a division operator and the denominator is > other multiplicand.
+      const Expr *rhse = rhs->IgnoreParenImpCasts();
+      if (const BinaryOperator *BOp = dyn_cast<BinaryOperator>(rhse)) {
----------------
zaks.anna wrote:
> I am not sure about this one.
> 
> We are saying that if the size expression in malloc was a multiplication of an expression and a constant (maxVal), than we should not warn if the expression was a devision of something unknown by another constant (val) that is greater or equal to the first constant (maxVal).
> 
> We don't know what that expression is and what the lhs of the devision is...
> 
> Other comments in regards to this check: the names used to represent the constants are not very expressive and there is quite a bit of copy and paste from the function above. The test only tests the case where val is equal to maxVal.
> 
I have added more comments for clarity. Essentially if:
x = a/b; // where n < b
malloc (x*n); //Then x*n will not overflow.

With regards to copy paste, I'm not sure about how to do this in a different way.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp:319
@@ -245,3 +318,3 @@
           if (!FD)
-            return;
+            continue;
 
----------------
zaks.anna wrote:
> Could you add a test case for this change and the one below?
> This should probably be bart of a separate commit as this is unrelated to the other change.
I tried to find a test case but couldn't. If you want i can submit this as a separate patch without a test case.


http://reviews.llvm.org/D9924