Page MenuHomePhabricator

Discard malloc-overflow bug-report when a known size is malloc'ed.
ClosedPublic

Authored by hiraditya on May 21 2015, 4:21 PM.

Details

Summary

This patch ignores malloc-overflow bug in two cases:
Case1:
x = a/b; where n < b
malloc (x*n); Then x*n will not overflow.

Case2:
x = a; // when 'a' is a known value.
malloc (x*n);

Also replaced isa with dyn_cast.

Reject multiplication by zero cases in MallocOverflowSecurityChecker
Currently MallocOverflowSecurityChecker does not catch cases like:
malloc(n * 0 * sizeof(int));

This patch rejects such cases.

Two test cases added. malloc-overflow2.c has an example inspired from a code
in linux kernel where the current checker flags a warning while it should not.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya updated this revision to Diff 26284.May 21 2015, 4:21 PM
hiraditya retitled this revision from to Ignore report when the argument to malloc is assigned known value.
hiraditya updated this object.
hiraditya edited the test plan for this revision. (Show Details)
hiraditya added a subscriber: Unknown Object (MLST).
zaks.anna added inline comments.Jun 17 2015, 10:34 PM
test/Analysis/malloc-overflow.c
117 ↗(On Diff #26284)

What is wrong with the current behavior of f15()?

ex.c:12:10: warning: Call to 'malloc' has an allocation size of 0 bytes
return malloc(n * 0 * sizeof(int));  // no-warning
hiraditya added inline comments.Jun 19 2015, 3:59 AM
test/Analysis/malloc-overflow.c
118 ↗(On Diff #26284)

This warning is valid. I'll update the test case. I only intended to ignore static analyzer warning related to memory leak. Thanks for the review.

hiraditya updated this revision to Diff 28077.Jun 20 2015, 6:47 AM

Updated test case.

mcrosier resigned from this revision.Aug 5 2015, 8:09 AM
mcrosier removed a reviewer: mcrosier.
zaks.anna edited edge metadata.Aug 6 2015, 10:56 AM

Thanks! See the comments inline.

lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
182 ↗(On Diff #28077)

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?)

186 ↗(On Diff #28077)

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.

319 ↗(On Diff #28077)

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.

hiraditya added inline comments.Aug 12 2015, 12:21 AM
lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
182 ↗(On Diff #28077)

Done.

186 ↗(On Diff #28077)

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.

319 ↗(On Diff #28077)

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.

hiraditya updated this revision to Diff 31904.Aug 12 2015, 12:24 AM
hiraditya edited edge metadata.

Added comments and changed the variable name.

hiraditya updated this revision to Diff 31913.Aug 12 2015, 2:35 AM

Added free, to avoid an unrelated warning.

x = a/b; where n < b
malloc (x*n); Then x*n will not overflow

I am not convinced that the new rule is strong enough. 'a' can be any expression. For example, maybe you have (b-1)*a/b and the denominator cancels out something unrelated to 'n' in the numerator? Maybe we could change the rule to "where n==b"? By the way, that is the only subcase that is being tested.

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

I suggest to experiment with refactoring out common parts into subroutines.

zaks.anna requested changes to this revision.Aug 17 2015, 12:29 PM
zaks.anna edited edge metadata.
This revision now requires changes to proceed.Aug 17 2015, 12:29 PM
hiraditya added a subscriber: hiraditya.EditedAug 17 2015, 1:18 PM

Date: Mon, 17 Aug 2015 19:29:29 +0000
To: hiraditya@msn.com; jordan_rose@apple.com; kremenek@apple.com; daniel.marjamaki@evidente.se; mclow.lists@gmail.com; adasgupt@codeaurora.org; zaks.anna@gmail.com
From: zaks.anna@gmail.com
CC: cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D9924: Ignore report when the argument to malloc is assigned known value

zaks.anna added a comment.

x = a/b; where n < b

malloc (x*n); Then x*n will not overflow

I am not convinced that the new rule is strong enough. 'a' can be any expression. For example, maybe you have (b-1)*a/b and the denominator cancels out something unrelated to 'n' in the numerator? Maybe we could change the rule to "where n==b"? By the way, that is the only subcase that is being tested.

Please correct me if I'm wrong.
My point was, as long as n<b' n*x would not overflow unless a' (the numerator) overflows in the first place.
Assuming a' does not overflow, a/b' would not overflow as well, since this is an integer division.

and since, a/b < a/n
which implies, x*n < a which does not overflow.

Maybe, I should add a check that `a, b, n' are positive.
So, in this case static analyzer can choose to be strict and reject false positives.

If a' might overflow, then in this case we can emit warning stating that the overflow is caused because a' might overflow.

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

I suggest to experiment with refactoring out common parts into subroutines.

Thanks, I'll try to refactor parts of it.
-Aditya

http://reviews.llvm.org/D9924

Maybe, I should add a check that `a, b, n' are positive.
So, in this case static analyzer can choose to be strict and reject false positives.

What would this buy us? Does the checker warn on underflow?

If a' might overflow, then in this case we can emit warning stating that the overflow is caused because a' might overflow.

I see your point now! I think we should improve the diagnostic that is produced in this case!

hiraditya updated this revision to Diff 32381.Aug 18 2015, 12:13 AM
hiraditya edited edge metadata.

Refactored the code and check whether denominator > 0.

Please review my patch. Thanks.

hiraditya updated this revision to Diff 34323.Sep 9 2015, 5:59 AM
hiraditya edited edge metadata.

Rebase with latest changes.

If a' might overflow, then in this case we can emit warning stating that the overflow is caused because a' might overflow.

I see your point now! I think we should improve the diagnostic that is produced in this case!

How was the following comment addressed? I do not see any tests for improved/changed diagnostics...

hiraditya updated this revision to Diff 34525.Sep 10 2015, 7:31 PM

Emit warning when numerator is unknown (may overflow).

The checker will ignore warning when numerator and denominator are both
known.
Added test case.
Format patch with clang-format.

Hi Anna,
I have updated the patch to address the changes you asked for.

zaks.anna accepted this revision.Sep 17 2015, 6:48 PM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Sep 17 2015, 6:48 PM

Aditya, can you update the patch title and summary to a commit message so I can commit it? Thanks!

hiraditya updated this revision to Diff 35461.Sep 22 2015, 8:49 PM
hiraditya edited edge metadata.

Updated commit message and summary.

hiraditya retitled this revision from Ignore report when the argument to malloc is assigned known value to Discard malloc-overflow bug-report when a known size is malloc'ed..Sep 22 2015, 8:51 PM
hiraditya updated this object.
This revision was automatically updated to reflect the committed changes.