[PATCH] D9924: Ignore report when the argument to malloc is assigned known value
Aditya Kumar via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 12 00:21:50 PDT 2015
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
More information about the cfe-commits
mailing list