Closed Bug 780778 Opened 7 years ago Closed 7 years ago

Make sure we always release stuff on the right thread.

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-track-main17+])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Since some of the StructuredClone*Info structs can hold onto CCd objects, we need to be sure we always empty them on the main thread.
Attachment #649478 - Flags: review?(bent.mozilla)
Comment on attachment 649478 [details] [diff] [review]
Patch

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

::: dom/indexedDB/IDBCursor.cpp
@@ -79,5 @@
>    }
>  
>    ~ContinueHelper()
>    {
> -    IDBObjectStore::ClearStructuredCloneBuffer(mCloneReadInfo.mCloneBuffer);

We still need this here and in all the other destructors (if something fails and we don't actually dispatch to the threadpool then we still end up never calling ReleaseMainThreadObjects).
If that happens, the only safe thing to do is to leak all the StructuredCloneFiles ... perhaps we should just abort?
No, if that happens then we're already on the main thread and the destructors for those objects will release everything just fine.
Attachment #649478 - Flags: review?(bent.mozilla) → review-
Attached patch PatchSplinter Review
Attachment #649478 - Attachment is obsolete: true
Attachment #666658 - Flags: review?(bent.mozilla)
Attachment #666658 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5c2b826588d3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 666658 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 661877
User impact if declined: Potential security problem
Testing completed (on m-c, etc.): On m-c, simple enough to understand easily.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.
Attachment #666658 - Flags: approval-mozilla-aurora?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> [Approval Request Comment]
> User impact if declined: Potential security problem

I'm assuming this isn't an sec-high or sec-critical since this landed on central without sec-approval. Sec-moderate?
That's probably reasonable.  This would be really hard to exploit.
Keywords: sec-moderate
Comment on attachment 666658 [details] [diff] [review]
Patch

[Triage Comment]
Approving for Aurora 17 - please land early on Monday to make the merge.
Attachment #666658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 661877
Keywords: regression
Whiteboard: [adv-track-main17+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.