[cfe-dev] [analyzer] Tracking values for generating bug reports

[Kristóf Umann via cfe-dev]

What are your feelings on this run? I'm personally pleased with how things
look like, and the only thing I'd like to add is pruning operator!=, though
I do not plan on doing any more analyses.

On Sat, 10 Aug 2019 at 03:48, Artem Dergachev <noqnoqneo at gmail.com> wrote:

>
>
> On 8/8/19 2:05 PM, Kristóf Umann wrote:
>
>
> The negatives:
> 1. Extra notes to functions following this pattern were very-very common:
>
> bool a(int b) {
>   return b == 0;
>   // Assuming 'b' is equal to 0
>   // Returning the value 1, which will be (a part of a) condition
> }
>
> void f(int b) {
>   int *x = 0;
>   if (a(b)) // call to 'a', returning from 'a'
>     *x = 5; // nullptr dereference
> }
>
> These notes are totally meaningless, and they aren't pruned out by [1]
> because the assumption notes are non-prunable (they explain a variable, b
> in this case, that is marked as interesting, as the return value is tracked
> by ReturnVisitor). A possible solution would be to prune linear functions
> in ConditionBRVisitor as well similarly to [1], but then we'd lose the
> assumption note. Instead, moving the non-prunable assumption note to the
> actual collapse point would be better, but a quick look at the generated
> ExplodedGraph reveals that this no longer an bug report construction issue:
> the collapse point is in fact at the return statement, rather then the
> condition.
>
> So, I think the bug report should always explain how the analyzer thinks.
> Ideally, this should be identical to how the bug could be reproduced, but
> if it is not (in this case the assumption should be made when a(b) is the
> condition), maybe the logic of the analysis should be changed instead.
>
> Example: Notes 19-23,
> http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8c536e75492d2588898e03178a704efb&report=12672&subtab=8c536e75492d2588898e03178a704efb
>
>
> This is due to the eagerly-assume behavior. The state split does actually
> happen at == or ! rather than at the if-statement.
>
> I guess you could check if the node is wearing an "ExprEngine: Eagerly
> Assume" tag.
>
>
Then I guess we should leave this as-is.

> 2. I didn't come across this too often, but it happened:
>
> int getInt(); int getInt2();
>
> bool a() {
>   return getInt2() && getInt();
>   // Returning value, which will be (a part of a) condition
> }
>
> void f() {
>   int *x = 0;
>   if (a()) // call to 'a', returning from 'a'
>     *x = 5; // nullptr dereference
> }
>
> This note wasn't pruned because a() has in fact more than 3 basic blocks,
> but it really doesn't change anything. I guess we *could* prune it, but I'm
> leaning on accepting it as a sacrifice.
>
> Example: Notes 65-68,
> http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8cc2c55ab79efc36fcd2b1725648c2c3&report=12678&subtab=8cc2c55ab79efc36fcd2b1725648c2c3
>
>
> I guess it depends:
>
> - If we're assuming that the condition is true, and the function returns A
> && B, the note is indeed redundant.
> - If we're assuming that the condition is true, and the function returns A
> || B, it's worth mentioning whether we think that A is true or we think
> that B is true.
> - If we're assuming that the condition is false, and the function returns
> A && B, it's worth mentioning whether we think that A is false or we think
> that B is false.
> - If we're assuming that the condition is false, and the function returns
> A || B, the note is indeed redundant.
>

Yea, I may fix it up later, though I'm not *immediately* worried about it
since this problem isn't condition tracking specific.

>
> 4. Calls to operator!= in foreach loops -- Are these necessary? Is there
> any, even articial cases where this is actually meaningful? This is pretty
> much the only non-constructive addition of notes that are introduced only
> because of condition tracking.
>
> Example: Notes 13-17, 22-26, 37-41:
> http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=50cc2bed223dc4ba62f46d0665346a5e&report=12743&subtab=50cc2bed223dc4ba62f46d0665346a5e
>
>
> Mmm, yeah, i'd rather skip them. We don't make much sense out of iterators
> anyway. I wish IteratorChecker could provide us with better notes.
>
>
I guess I'll fix this up quickly before making this on-by-default.

> Positives:
>
> In general, the wording as the analyses were run may be imperfect, but
> they indicate what the message is about very precisely. The path of
> execution is much better explained, and even if the extra function call
> isn't meaningful, the note messages state clearly that we're only talking
> about a condition, I can skip them without being bothered by it too much. I
> am given extra information without the cost of polluting the rest of the
> report.
>
> Here is a collection of my favorites, though I found most of the extra
> notes nice:
>
> 1. Here, we encounter a nullpointer dereference issue. Op0 is assumed to
> be equal to Op1, but only due to condition tracking do we explain that Op1
> is believed to be null, and THAT is why this assumption (Op1 == Op0) is so
> important!
>
> Notes 23-31:
> http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=6b5f3e43213b64eaef4a25a249c59baf&report=12955&subtab=6b5f3e43213b64eaef4a25a249c59baf
>
>
> The note is indeed extremely helpful, but the warning itself should have
> been suppressed by the "inlined defensive check" heuristic. I guess the
> heuristic doesn't know how to carry over to the other side of the `==`.
>
>
> 3. This one is weird, I'm not yet sure how this note came about, but yet
> again, it isn't clear AT ALL in the original report why CountS will be a
> source of a null dereference error. The extra note tells us that CountS is
> non-null, but its pointee is.
>
> Note 19:
> http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=356d88cb062474fdf17584bf1451bffc&report=13412&subtab=356d88cb062474fdf17584bf1451bffc
>
>
> You're talking about the "Assuming pointer value is null" thingy? I
> suspect that it's a bug and the note is in fact meaningless. See also
> https://reviews.llvm.org/D65889?id=214186#inline-590619 - i've seen them
> before but i suspect that i suddenly see more of them these days.
> Generally, i still don't understand the report after seeing these notes.
> Even if the notes tell anything about the pointee, the code they're
> attached to doesn't.
>
> I also don't understand why did we bring in the copy-constructor. Was
> there an invalidation of all heap going on that also touched *CountS?
>

The answer lies in the DEBUG notes run: We already track CollectionS to the
point where it was initialized, and FindLastStoreBRVisitor tracks the right
hand side of it. This will trigger control dependency calculation again,
resulting in State being tracked, which leads to that monster of a copy
constructor call, where we explain why we believed State to be null. The
call to IntrusiveRefCntPtr::operator bool was pruned, but it explains why
we placed a note about Obj, because thats what it returns. I do wonder
however why it wasn't accompanied with ", which will be (a part of a)
condition", I'll take a look.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190810/55be8540/attachment.html>