Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsDeviceMotion::DeviceMotionChanged may index out of bounds mWindowListeners array

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: smaug, Assigned: dougt)

Tracking

12 Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox10- wontfix, firefox11+ wontfix, firefox12+ fixed, firefox13+ fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [sg:critical][qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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).
Whiteboard: [sg:critical]
We should probably protect mListeners here as well.
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?
Assignee: nobody → doug.turner
status-firefox10: --- → wontfix
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox10: --- → -
tracking-firefox11: --- → -
tracking-firefox12: --- → +
tracking-firefox13: --- → +
status-firefox-esr10: --- → affected
Keywords: testcase-wanted
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.
(Assignee)

Comment 4

5 years ago
Created attachment 604352 [details] [diff] [review]
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.
Attachment #604352 - Flags: review?(bugs)
(Assignee)

Comment 5

5 years ago
i see a few crashes in crash stats that are probably this bug.
(Reporter)

Comment 6

5 years ago
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> >
Attachment #604352 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

5 years ago
Created attachment 604416 [details] [diff] [review]
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.
Attachment #604352 - Attachment is obsolete: true
Attachment #604416 - Flags: review?(bugs)
(Reporter)

Comment 10

5 years ago
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.
Attachment #604416 - Flags: review?(bugs) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 604419 [details] [diff] [review]
patch v.3
Attachment #604416 - Attachment is obsolete: true
Attachment #604419 - Flags: review?(bugs)
(Reporter)

Comment 12

5 years ago
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.
Attachment #604419 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1f94ebd302
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6737b6762eb8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

5 years ago
Created attachment 604713 [details] [diff] [review]
patch of what landed on m-c
Attachment #604713 - Flags: review+
Attachment #604713 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 16

5 years ago
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.
tracking-firefox11: - → ?
(Assignee)

Comment 17

5 years ago
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.
Can we get a risk evaluation of this bug before uplifting to Aurora 12, and therefore Beta 12 in a couple of days?

Updated

5 years ago
tracking-firefox-esr10: --- → ?
tracking-firefox11: ? → +
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.
Attachment #604713 - Flags: approval-mozilla-beta+
Attachment #604713 - Flags: approval-mozilla-aurora?
Attachment #604713 - Flags: approval-mozilla-aurora-
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/0209c4988d0d
status-firefox12: affected → fixed
status-firefox13: affected → fixed

Updated

5 years ago
tracking-firefox-esr10: ? → 12+
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.
Attachment #604713 - Flags: approval-mozilla-esr10+
qa- until testcase-wanted is satisfied
Whiteboard: [sg:critical] → [sg:critical][qa-]
https://hg.mozilla.org/releases/mozilla-esr10/rev/a288423859c5
status-firefox-esr10: affected → fixed
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.