1330306 - [Static Analysis][Out-of-bounds access] In function LongestPhaseSelfTime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that an out-of-bounds access will occur in this following portion of the code:

>>    for (size_t i = 0; i < PHASE_LIMIT; ++i) {
>>        Phase parent = phases[i].parent;
>>        if (parent == PHASE_MULTI_PARENTS) {
>>            // Subtract out only the time for the children specific to this
>>            // parent.
>>            for (auto edge : dagChildEdges) {
>>                if (edge.parent == parent) {
>>                    size_t dagSlot = phaseExtra[edge.parent].dagSlot;
>>                    CheckSelfTime(parent, edge.child, times, selfTimes, times[dagSlot][edge.child]);
>>                    MOZ_ASSERT(selfTimes[parent] >= times[dagSlot][edge.child]);
>>                    selfTimes[parent] -= times[dagSlot][edge.child];
>>                }
>>            }
>>        } else if (parent != PHASE_NO_PARENT) {
>>            MOZ_ASSERT(selfTimes[parent] >= selfTimes[i]);
>>            CheckSelfTime(parent, Phase(i), times, selfTimes, selfTimes[i]);
>>            selfTimes[parent] -= selfTimes[i];
>>        }

1. size_t dagSlot = phaseExtra[edge.parent].dagSlot; since this translates to size_t dagSlot = phaseExtra[PHASE_MULTI_PARENTS].dagSlot, but phaseExtra has only PHASE_LIMIT
2. In this function  CheckSelfTime(parent, edge.child, times, selfTimes, times[dagSlot][edge.child])
3. selfTimes[parent] -= times[dagSlot][edge.child];

But looking to the definition of dagChildEdges we see that there is no element having the parent = PHASE_MULTI_PARENTS, thus that portion of the code becomes dead. 

What i propose is to eliminate the entire if branch and to leave only the else branch that will qualify as an if branch.

>>       if (parent != PHASE_NO_PARENT) {
>>            MOZ_ASSERT(selfTimes[parent] >= selfTimes[i]);
>>            CheckSelfTime(parent, Phase(i), times, selfTimes, selfTimes[i]);
>>            selfTimes[parent] -= selfTimes[i];
>>        }

We do this only to silence this checker.

Comment 1

[Andi [:andi]]

Attachment: Bug 1330306 - silence sa-checker regarding false-positive out-of-bounds access/read.

Review commit: https://reviewboard.mozilla.org/r/103884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103884/

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8825808 [details]
Bug 1330306 - silence sa-checker regarding false-positive out-of-bounds access/read.

Steve should probably look at this.

It would be great if we could simplify/remove this code - I thought we had a bug on file for that but I can't find it right now.

Comment 3

[Steve Fink [:sfink] [:s:]]

We don't need to change this to silence the checker; the checker is absolutely correct. The logic is bad.

The edge.parent == parent test is completely bogus, it should be edge.parent == i. And then all instances of 'parent' should be edge.parent. Because I messed up the previous check, I reasoned that edge.parent and parent are the same, so I may as well use the shorter one. But it's completely wrong; parent is PHASE_MULTI_PARENTS, which is out of bounds in all the things being indexed. edge.parent is what I want.

Jon is correct in that the correct fix is to rewrite the phase dag code so that it instantiates a simple tree, rather than having these two data structures (phase[].parent and dagChildEdges) that together encode the full tree. I still have the code where I began that rewrite, but it looks like I never filed a bug. Filed bug 1361458 for that.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Fix self-time computation for multiparented phases

I updated the comments to make things a little more clear, but the real fix here is bug 1361458. I'd like to patch this up for now, though.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8863849 [details] [diff] [review]
Fix self-time computation for multiparented phases

Review of attachment 8863849 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, that explains what's going wrong here.

It would be really great to have tests to prove that this works but let's just fix this for now.

Thanks for adding the comments.

::: js/src/gc/Statistics.cpp
@@ +972,5 @@
>                  }
>              }
>          } else if (parent != PHASE_NO_PARENT) {
>              MOZ_ASSERT(selfTimes[parent] >= selfTimes[i]);
>              CheckSelfTime(parent, i, times, selfTimes, selfTimes[i]);

Can you swap the previous two lines so that CheckSelfTime will print useful information before the assert goes off?  Possibly the assert should be in CheckSelfTime.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69ac2411d01f
Fix self-time computation for multiparented phases, r=jonco

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69ac2411d01f

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 53, fairly late in the 54 cycle for uplift to beta.
Steve, what do you think, is this worth the risk to uplift? Or should we let it stay in 55?

Comment 10

[Steve Fink [:sfink] [:s:]]

It's an out of bounds write, which is a little scary from a stability standpoint. But it seems unlikely to get into anything very important, and this only affects statistics. The patch is pretty darn simple, so it doesn't seem very risky. I'm not sure I have a good sense for what warrants an uplift. I'm fine with either outcome; if it's a nuisance or more overhead in terms of things to worry about, feel free to wontfix for 54. If it were early beta, I'd probably vote for uplifting it.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's go with 55, we do want to minimize risk for 54 at this point. Just checking if you felt strongly.