The default bug view has changed. See this FAQ.

Workers + Files exposes threadsafety assertions with DataOwner

RESOLVED FIXED in Firefox 9

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla9
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8- wontfix, firefox9+ fixed, firefox10+ fixed, blocking1.9.2 -, status1.9.2 unaffected)

Details

(Whiteboard: [sg:high][qa?])

Attachments

(1 attachment, 2 obsolete attachments)

Jonas reports that bent has seen threadsafety assertions from DataOwner when manipulating File objects in Workers in certain ways.
Yeah, we need to make the DataOwner refcounting threadsafe. As things stand now, if a memory-blob is used on both the main thread and a worker thread, the refcount can end up both too high and too low due to races.
Assignee: nobody → khuey
Created attachment 572786 [details] [diff] [review]
Patch
Attachment #572786 - Flags: review?(jonas)
Attachment #572786 - Flags: review?(benjamin)
Created attachment 572820 [details] [diff] [review]
Patch
Attachment #572786 - Attachment is obsolete: true
Attachment #572786 - Flags: review?(jonas)
Attachment #572786 - Flags: review?(benjamin)
Attachment #572820 - Flags: review?(jonas)
Attachment #572820 - Flags: review?(benjamin)
Comment on attachment 572820 [details] [diff] [review]
Patch

Err.. you only want to delete if the refcount goes to 0, right?
Attachment #572820 - Flags: review?(jonas) → review-
Created attachment 572824 [details] [diff] [review]
Patch
Attachment #572820 - Attachment is obsolete: true
Attachment #572820 - Flags: review?(benjamin)
Attachment #572824 - Flags: review?(jonas)
Attachment #572824 - Flags: review?(jonas) → review+
Attachment #572824 - Flags: review?(benjamin)
If we're respinning 8.0, perhaps we should consider including this?  It's a pretty easy fix to a new bug in 8.0.
tracking-firefox10: --- → ?
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?

Comment 7

5 years ago
[Triage Comment]
This is sg:high and therefore doesn't meet the criteria for landing on the release branch for a chemspill.
tracking-firefox8: ? → -
Review ping.
Attachment #572824 - Flags: review?(benjamin) → review+
Wait... I don't like that this doesn't stabilize the refcount. Having inconsistent implementations of Release seems like a recipe for headaches, and having no stabilization seems like a recipe for double-free.
refcount stabilization was a terrible idea. We should assert if you try to addref a dying object, not propagate stabilization to new macros where it's not already potentially used.
Fatally asserting sounds fine, but I imagine we'd only do that in debug builds since you have to have some other state bit to track that. For opt builds we'd just have no protection against double frees?
You really don't need another state bit, just use PR_UINT32_MAX as a sentinel value. I'm not sure that the extra branch is worth the protection in release builds, but we should probably measure to see how much it actually costs.
Checked in without a test case.

https://hg.mozilla.org/mozilla-central/rev/d20b9d389776
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment on attachment 572824 [details] [diff] [review]
Patch

Requesting aurora and beta approval for this security regression introduced in Firefox 8.
Attachment #572824 - Flags: approval-mozilla-beta?
Attachment #572824 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox8: affected → wontfix
tracking-firefox10: ? → +
tracking-firefox9: ? → +

Comment 15

5 years ago
Comment on attachment 572824 [details] [diff] [review]
Patch

[triage comment]
Please land on aurora and beta today if at all possible.
Attachment #572824 - Flags: approval-mozilla-beta?
Attachment #572824 - Flags: approval-mozilla-beta+
Attachment #572824 - Flags: approval-mozilla-aurora?
Attachment #572824 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a45dd5ac1f90
https://hg.mozilla.org/releases/mozilla-beta/rev/6858e50813de
status-firefox10: affected → fixed
status-firefox9: affected → fixed
Target Milestone: mozilla11 → mozilla9
Does this affect the 1.9.2 branch? (qawanted)
blocking1.9.2: --- → ?
status1.9.2: --- → ?
Keywords: qawanted
No, File objects are main thread only in 1.9.2.
status1.9.2: ? → unaffected
Keywords: qawanted
blocking1.9.2: ? → -
Is there something QA can do to verify this fix?
Whiteboard: [sg:high] → [sg:high][qa?]
Group: core-security
The tests for this landed in Bug 698621 for reasons that I don't understand.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.