Closed
Bug 878703
Opened 12 years ago
Closed 12 years ago
Cleanup usage of IO thread only objects
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: janv, Assigned: janv)
References
Details
(Keywords: csectype-race, sec-moderate, Whiteboard: [adv-main23+])
Attachments
(2 files)
(deleted),
patch
|
bent.mozilla
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
mFileManagerInfos member in IndexedDatabaseManager supposed to be touched only on the IO thread, unfortunately it is not.
This patch fixes it and also adds AssertIsOnIOThread() to all IO thread only methods to make the thread they are running on more obvious.
Temp storage implementation depends on this fix.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Attachment #757241 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #757241 -
Attachment is patch: true
Comment on attachment 757241 [details] [diff] [review]
patch v0.1
Review of attachment 757241 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! r=me with just a few changes:
::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +644,5 @@
>
> return NS_OK;
> }
> +
> +GetFileReferencesHelper::GetFileReferencesHelper(const nsACString& aOrigin,
Nit: Just inline this
@@ +659,5 @@
> +{
> +}
> +
> +nsresult
> +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt,
Assert that you start out on the main thread here.
::: dom/indexedDB/IndexedDatabaseManager.h
@@ +96,5 @@
> AsyncDeleteFile(FileManager* aFileManager,
> int64_t aFileId);
>
> + nsresult
> + GetFileReferences(const nsACString& aOrigin,
Nit: Make this 'BlockAndGetFileReferences', and add a big scary comment saying that this is to be used in testing-only code because it blocks the main thread.
Attachment #757241 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This is a thread-unsafe access of a hash table. s-s for now until someone from the sec team can rate it.
Group: core-security
Comment on attachment 757241 [details] [diff] [review]
patch v0.1
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unclear
User impact if declined: Crashes, maybe an exploit. Depends on a race so it might be tough.
Testing completed (on m-c, etc.): It's been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): No risks that we're aware of.
String or IDL/UUID changes made by this patch: None
Attachment #757241 -
Flags: approval-mozilla-beta?
Attachment #757241 -
Flags: approval-mozilla-aurora?
Comment 7•11 years ago
|
||
(In reply to ben turner [:bent] from comment #6)
> User impact if declined: Crashes, maybe an exploit. Depends on a race so it
> might be tough.
Can we get more confidence than maybe an exploit? If so, can we make this a sec-critical.
> Risk to taking this patch (and alternatives if risky): No risks that we're
> aware of.
Low enough risk for the final beta of a release?
status-firefox24:
--- → fixed
tracking-firefox22:
--- → ?
tracking-firefox23:
--- → +
tracking-firefox24:
--- → +
Flags: needinfo?(bent.mozilla)
Updated•11 years ago
|
Attachment #757241 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•11 years ago
|
||
status-firefox22:
--- → affected
status-firefox23:
--- → fixed
(In reply to Alex Keybl [:akeybl] from comment #7)
> Can we get more confidence than maybe an exploit? If so, can we make this a
> sec-critical.
We'd need someone from the sec team to weigh in on this.
> Low enough risk for the final beta of a release?
I'm not sure, so I guess not :(
Flags: needinfo?(bent.mozilla)
Comment 10•11 years ago
|
||
Did this affect 21 or earlier, such as ESR17?
Also, before this went in initially, it should have gone through sec-approval and gotten a rating, per https://wiki.mozilla.org/Security/Bug_Approval_Process.
I'm not sure of the rating here. Asking Dan.
Flags: needinfo?(dveditz)
Updated•11 years ago
|
Comment 11•11 years ago
|
||
Comment on attachment 757241 [details] [diff] [review]
patch v0.1
We're not confident enough in the fix (even if this is sec-crit) to take in a final beta.
Attachment #757241 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•11 years ago
|
Flags: needinfo?(dveditz)
Keywords: csec-race,
sec-moderate
Updated•11 years ago
|
Whiteboard: [adv-main23+]
Updated•11 years ago
|
status-firefox-esr17:
--- → wontfix
status-firefox-esr24:
--- → unaffected
Updated•11 years ago
|
Updated•11 years ago
|
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•