Closed Bug 878703 Opened 7 years ago Closed 7 years ago

Cleanup usage of IO thread only objects

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 + wontfix
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- wontfix
firefox-esr24 --- unaffected
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- unaffected

People

(Reporter: janv, Assigned: janv)

References

Details

(Keywords: csectype-race, sec-moderate, Whiteboard: [adv-main23+])

Attachments

(2 files)

Attached patch patch v0.1Splinter 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: nobody → Jan.Varga
Blocks: 785884
Status: NEW → ASSIGNED
Attachment #757241 - Flags: review?(bent.mozilla)
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+
Attached patch interdiffSplinter Review
https://hg.mozilla.org/mozilla-central/rev/23a34163cd70
Status: ASSIGNED → RESOLVED
Closed: 7 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?
(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?
Flags: needinfo?(bent.mozilla)
Attachment #757241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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)
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)
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-
Flags: needinfo?(dveditz)
Whiteboard: [adv-main23+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.