726502 - nsDeviceMotion::DeviceMotionChanged may index out of bounds mWindowListeners array

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

The code dispatches DOM events synchronously, and DOM event listeners can do anything, 
like close windows. As far as I see nothing guarantees that
mWindowListeners is still long enough when next iteration starts (after event dispatch).

Comment 1

[Johnny Stenback (:jst)]

We should probably protect mListeners here as well.

Comment 2

[Johnny Stenback (:jst)]

Doug, any chance you could look into this? If you're too pressured with mobile stuff right now, do you know someone else who could look into this?

Comment 3

[(not currently active) Ted Mielczarek]

Presumably a testcase would be something like:
Have a page with an iframe. The iframe adds an event listener for "devicemotion" the notifies the parent, at which time the parent adds an event listener for "devicemotion". In the parent's "devicemotion" handler function, close the window. This should result in nsDeviceMotion touching freed memory. (Presumably we could validate this with valgrind.)

I guess this might be easier if the parent page was itself an iframe, so it could just remove itself from the parent document or something instead of relying on being able to close the window.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.1

I couldn't reproduce any failure with the code as it is.  My test case was to create 100 iframe each using device motion.  In the device motion callback, the iframe would ask the parent document (top) to randomly remove one of the iframes.  The test would complete when all of the iframes were removed.  I ran this test a few hundred times without any failure. 

Ted, is this similar to what you were thinking?


I do not see any harm in making a copy of the listeners before calling out through the callback.  This is what the attach patch does.  We do not need any locking because all of this code is main thread only.

Comment 5

[Doug Turner (:dougt)]

i see a few crashes in crash stats that are probably this bug.

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 604352 [details] [diff] [review]
patch v.1


>+  nsCOMArray<nsIDeviceMotionListener> listeners = mListeners;
>+  for (PRUint32 i = listeners.Count(); i > 0 ; ) {
>     --i;
>     nsRefPtr<nsDeviceMotionData> a = new nsDeviceMotionData(type, x, y, z);
>-    mListeners[i]->OnMotionChange(a);
>+    listeners[i]->OnMotionChange(a);
>   }
> 
>-  for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+  nsTArray<nsIDOMWindow*> windowListeners = mWindowListeners;
Nothing keeps the objects in windowListeners alive.
You need to make an nsCOMArray<nsIDOMWindow> or nsTArray<nsCOMPtr<nsIDOMWindow> >

Comment 7

[Doug Turner (:dougt)]

okay.  But I am not seeing ~() being called when RemoveWindowListener is called.  Something is keeping the window alive.  Maybe we just don't have the right test case.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

I'm sure Jesse or mw22 could create you a testcase where window object is deleted ;)
But since the bug is rather obvious here, I wouldn't spend time for the testcase.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch .2

I don't remember why we weren't using nsCOMArray<nsIDOMWindow>.  I think it was a shutdown leak.  I pushed this patch to try.

https://hg.mozilla.org/try/rev/e48f12163d10


Alternatively we can just use a nsCOMArray in nsDeviceMotion::DeviceMotionChanged.

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 604416 [details] [diff] [review]
patch .2

This leads most certainly to leaks
(which lead to huge cycle collection times).

Could you just copy the values in
nsTArray<nsIDOMWindow*> mWindowListeners;
to nsTArray<nsCOMPtr<nsIDOMWindow> > and then iterate that.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 604419 [details] [diff] [review]
patch v.3

>-  for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+  nsCOMArray<nsIDOMWindow> windowListeners;
>+  for (PRUint32 i = windowListeners.Count(); i > 0 ; ) {
>+      windowListeners.AppendObject(mWindowListeners[i]);
>+  }
2 space indentation, please.

Comment 13

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1f94ebd302

Comment 14

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/6737b6762eb8

Comment 15

[Doug Turner (:dougt)]

Attachment: patch of what landed on m-c

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Argh, I didn't review the commit message. I guess I should have.
We don't want commit messages to say "hey, here is a possible sg:crit" bug. 

Could we take the patch to FF11, if we're going to fix that Pwn2Own bug too.

Comment 17

[Doug Turner (:dougt)]

i think it is pretty obvious to look at csets that have bugs that are marked as Security-Sensitive.  Check-in comments should not lie.

Comment 18

[Alex Keybl [:akeybl]]

Can we get a risk evaluation of this bug before uplifting to Aurora 12, and therefore Beta 12 in a couple of days?

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 604713 [details] [diff] [review]
patch of what landed on m-c

[Triage Comment]
Approving this sg:crit fix for Beta 12 since Aurora is now 13.

Comment 20

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0209c4988d0d

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 604713 [details] [diff] [review]
patch of what landed on m-c

[Triage Comment]
Approving for landing on ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.

Comment 22

[u279076]

qa- until testcase-wanted is satisfied

Comment 23

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/a288423859c5