Closed Bug 1330306 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- wontfix
firefox54 --- fix-optional
firefox55 --- fixed

People

(Reporter: andi, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1398289, CID 1398288, 1398287.)

Attachments

(2 files)

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 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.
Flags: needinfo?(sphink)
Attachment #8825808 - Flags: review?(jorendorff)
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.
Flags: needinfo?(sphink)
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.
Attachment #8863849 - Flags: review?(jcoppeard)
Assignee: bpostelnicu → sphink
Status: NEW → ASSIGNED
Duplicate of this bug: 1338067
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.
Attachment #8863849 - Flags: review?(jcoppeard) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69ac2411d01f
Fix self-time computation for multiparented phases, r=jonco
https://hg.mozilla.org/mozilla-central/rev/69ac2411d01f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Flags: needinfo?(sphink)
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.
Flags: needinfo?(sphink)
Let's go with 55, we do want to minimize risk for 54 at this point. Just checking if you felt strongly.
You need to log in before you can comment on or make changes to this bug.