Crash in InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Events
--
critical
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: mccr8, Assigned: smaug)

Tracking

({crash, csectype-bounds, sec-moderate})

Trunk
mozilla51
x86
Windows 7
crash, csectype-bounds, sec-moderate
Points:
---
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox-esr45 wontfix, firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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:
  bp-96879694-cd7f-4036-8d85-5df8e2160901
(Reporter)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
I wonder... since this is a bit similar to that other mListeners related bug.
Could we have some bug in nsAutoTObserverArray?
(Assignee)

Comment 3

2 years ago
or are we using it in some wrong way.
(Reporter)

Comment 4

2 years ago
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.
(Reporter)

Comment 5

2 years ago
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.
(Reporter)

Updated

2 years ago
Crash Signature: [@ InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo] → [@ InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::EventListenerManager::GetListenerInfo ]
(Assignee)

Comment 6

2 years ago
Created attachment 8787704 [details] [diff] [review]
ELM fixes

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)
(Reporter)

Comment 7

2 years ago
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+
(Assignee)

Comment 8

2 years ago
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)
(Reporter)

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
Well, I don't still understand how the crashes might happen, so really hard to say.
(Reporter)

Comment 11

2 years ago
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.
Keywords: csectype-bounds, sec-moderate
(Reporter)

Updated

2 years ago
status-firefox48: --- → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox-esr45: --- → wontfix
(Assignee)

Comment 12

2 years ago
Created attachment 8787751 [details] [diff] [review]
safer_elm_for_devtools.diff

Let do only this change in this bug.
Attachment #8787704 - Attachment is obsolete: true
Attachment #8787751 - Flags: review?(continuation)
(Reporter)

Updated

2 years ago
Attachment #8787751 - Flags: review?(continuation) → review+
Group: core-security → dom-core-security
https://hg.mozilla.org/mozilla-central/rev/a213fcd04400
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Olli, should this be uplifted to Aurora50?
Flags: needinfo?(bugs)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8787751 [details] [diff] [review]
safer_elm_for_devtools.diff

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

Comment 17

2 years ago
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]
safer_elm_for_devtools.diff

Crash fix, seems to have worked in Nightly51, Aurora50+
Attachment #8787751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Updated

2 years ago
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+]

Updated

10 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.