FilePickerParent::FileSizeAndDateRunnable::~FileSizeAndDateRunnable() releases main thread objects off the main thread

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: billm)

Tracking

({csectype-race, sec-moderate})

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox36 disabled, firefox37 disabled, firefox38 disabled, firefox39 fixed, firefox-esr31 unaffected, firefox-esr38 disabled, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

4 years ago
I see a fair number of crashes like this on Nightly:

https://crash-stats.mozilla.com/report/index/e13e8124-1e55-4560-b40c-e13e92150317#allthreads

I'm not sure how bad of a security problem this is given that we're always crashing but maybe the code is otherwise trying to use these objects.
Reporter

Updated

4 years ago
Crash Signature: [@ NS_CycleCollectorSuspect3 ]
FileSizeAndDateRunnable (and anything that jumps around several threads) should be holding FileImpl, not nsIDOMFile.
Posted patch patchSplinter Review
I guess that's because FileImpl has threadsafe AddRef and Release?
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8579618 - Flags: review?(bent.mozilla)
Reporter

Comment 3

4 years ago
This looks like a regression from bug 910384.  How bad is this security-wise?
Comment on attachment 8579618 [details] [diff] [review]
patch

Review of attachment 8579618 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, FileImpl is the threadsafe (and potentially shared) piece for blobs. Blob/File are the DOM parts, and they're CC'd, etc.
Attachment #8579618 - Flags: review?(bent.mozilla) → review+
This is e10s-only, so it only affects Nightly.
Comment on attachment 8579618 [details] [diff] [review]
patch

Review of attachment 8579618 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/FilePickerParent.h
@@ +7,4 @@
>  #ifndef mozilla_dom_FilePickerParent_h
>  #define mozilla_dom_FilePickerParent_h
>  
>  #include "nsIDOMFile.h"

Oh, you might be able to lose this now.
Reporter

Comment 8

4 years ago
I'm going to mark this as moderate, because it seems like it would be pretty hard to exploit.  Note that we're hitting a release-mode assert here, so somehow you'd have to get this runnable to run some code before any CC-ed object is addrefed or released, plus whatever trickiness there is to exploit any race condition.  It would still be nice to get this fixed so we can find other threading issued without digging through all of the crashes for this signature.
https://hg.mozilla.org/mozilla-central/rev/565712db304c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Does this affect B2G at all?
Flags: needinfo?(wmccloskey)
No, I believe they have their own separate file picker.
Flags: needinfo?(wmccloskey)

Updated

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