Closed Bug 1300129 Opened 3 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo


(Core :: DOM: Events, defect, critical)

Windows 7
Not set



Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed
firefox51 --- fixed


(Reporter: mccr8, Assigned: smaug)



(Keywords: crash, csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main50+])

Crash Data


(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-79ac1c16-9a1b-4f2b-a4bf-6e2d12160902.

This crash is odd.

uint32_t count = mListeners.Length();
for (uint32_t i = 0; i < count; ++i) {
  const Listener& listener = mListeners.ElementAt(i);

The index is one more than the length. The mostly likely cause that I can think of is iterator invalidation, ie while we're iterating over the member variable of the ELM, we end up removing something from the array.

This is the only crash report I saw with a deep stack:
This signature is 44% of the invalid array index crashes in the last 7 days, with 37 crashes. It is the most common such crash.
I wonder... since this is a bit similar to that other mListeners related bug.
Could we have some bug in nsAutoTObserverArray?
or are we using it in some wrong way.
Oh, I didn't even notice that it was an auto observer array. So we would expect this to be okay, even in the presence of some iterator invalidation.
The comment on nsAutoTObserverArray::ElementAt() is:
  // This method provides direct access to an element of the array. The given
  // index must be within the array bounds. If the underlying array may change
  // during iteration, use an iterator instead of this function.

So I think the ElementAt is bad.
Crash Signature: [@ InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo] → [@ InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::EventListenerManager::GetListenerInfo ]
Attached patch ELM fixes (obsolete) — Splinter Review
Make RemoveEventListenerInternal safer by moving NotifyEventListenerRemoved outside the loop
(and fix also i handling). Iterators are hard to use here because we want to remove item and forward iterator can't quite deal that right now. And I want to keep doing forward iteration, not backwards.

Switch EventListenerManager::GetListenerInfo to use iterator in case CompileEventHandlerInternal causes changes to mListeners.

Pure guess fixes.
Assignee: nobody → bugs
Attachment #8787704 - Flags: review?(continuation)
Comment on attachment 8787704 [details] [diff] [review]
ELM fixes

Review of attachment 8787704 [details] [diff] [review]:

Thanks for fixing this.

::: dom/events/EventListenerManager.cpp
@@ +670,2 @@
>          --count;
> +        --i;

Hmm. This will underflow if i = 0, but I guess that's okay because this is an unsigned integer. It'll go back to 0 at the end of the loop.
Attachment #8787704 - Flags: review?(continuation) → review+
yes, I was thinking about that with i, and it should just work.

Hopefully the patch fixes something. 

since this doesn't have security rating, do you think I need approval.

I'd use commit message "Ensure devtools get the right event listener data" or something like that. vague, but true.
Flags: needinfo?(continuation)
(In reply to Olli Pettay [:smaug] from comment #8)
> since this doesn't have security rating, do you think I need approval.

It needs a rating before it can be landed. The rating would determine whether or not it needs approval. How bad of a security issue do you think this is? On Nightly and Aurora, it is less of a problem because we have these checks, but for older branches it might be a problem. I don't know how easily content could control these, and reading a few off the end of an array doesn't seem like it would do too much.
Flags: needinfo?(continuation)
Well, I don't still understand how the crashes might happen, so really hard to say.
Olli said that EventListenerService is the only thing that can trigger this method, and it can only be run from Chrome JS, so I'm just going to mark this sec-moderate, so it will not need sec-approval before landing.
Let do only this change in this bug.
Attachment #8787704 - Attachment is obsolete: true
Attachment #8787751 - Flags: review?(continuation)
Attachment #8787751 - Flags: review?(continuation) → review+
Group: core-security → dom-core-security
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Olli, should this be uplifted to Aurora50?
Flags: needinfo?(bugs)
Comment on attachment 8787751 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: possible crashes when using devtools
[Describe test coverage new/current, TreeHerder]: NA
[Risks and why]: Should be very safe, but since the crash isn't reproduceable, it isn't 100% sure whether this helps
[String/UUID change made/needed]: NA
Flags: needinfo?(bugs)
Attachment #8787751 - Flags: approval-mozilla-aurora?
I don't see this signature on Nightly any more, while it used to be the most common crash with InvalidArrayIndex_CRASH. I still see it on Nightly. So I think your patch worked.
Comment on attachment 8787751 [details] [diff] [review]

Crash fix, seems to have worked in Nightly51, Aurora50+
Attachment #8787751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1304596
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.