3.12 KB, patch
|Details | Diff | Splinter Review|
4.26 KB, patch
|Details | Diff | Splinter Review|
3.39 KB, patch
|Details | Diff | Splinter Review|
Bug 776505 involves an intermittent orange with ASSERTION: nsDeviceContext not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread(). roc landed an assertion in nsDeviceContext's constructor that it is being created on the main thread, but it does not seem to be triggering in that case. Given that, it seems that it is likely that there is some kind of memory corruption. If the field that stores the thread ID gets messed up, then we'll fail this assertion, even though it is only ever used on the main thread. The stacks seem to always look like: 0 XUL!nsDeviceContext::AddRef() [nsDeviceContext.h : 26 + 0x0] 1 XUL!nsRenderingContext::Init(nsDeviceContext*, gfxContext*) [nsAutoPtr.h : 845 + 0x7] 2 XUL!PresShell::RenderDocument(nsRect const&, unsigned int, unsigned int, gfxContext*) [nsPresShell.cpp : 4359 + 0xd] 3 XUL!mozilla::dom::CanvasRenderingContext2D::DrawWindow(nsIDOMWindow*, double, double, double, double, nsAString_internal const&, unsigned int, mozilla::ErrorResult&) [CanvasRenderingContext2D.cpp : 3962 + 0xb] 4 XUL!mozilla::dom::CanvasRenderingContext2DBinding::drawWindow [CanvasRenderingContext2DBinding.cpp : 1868 + 0x43] 5 XUL!mozilla::dom::CanvasRenderingContext2DBinding::genericMethod [CanvasRenderingContext2DBinding.cpp : 2570 + 0x12] This is the strip of code in question: 4347 nsDeviceContext* devCtx = mPresContext->DeviceContext(); 4348 gfxFloat scale = gfxFloat(devCtx->AppUnitsPerDevPixel())/nsPresContext::AppUnitsPerCSSPixel(); 4349 aThebesContext->Scale(scale, scale); 4354 aThebesContext->NudgeCurrentMatrixToIntegers(); 4356 AutoSaveRestoreRenderingState _(this); 4358 nsRefPtr<nsRenderingContext> rc = new nsRenderingContext(); 4359 rc->Init(devCtx, aThebesContext); Not knowing anything about this code, my spideysense did start tingling at that raw pointer to the devCtx. |this| strongly holds mPresContext, which strongly holds its device context, but I guess if either of those fields changes in between line 4347 and 4359 then we could have a UAF. To fix that problem, devCtx could become a strong pointer, or just be inlined, as it is only used twice. We could also add an assert that devCtx is the same as mPresContext->DeviceContext() before the Init, assuming that should be the case. Kyle also suggested the possibility that devCtx is just corrupt from the get-go. This could be tested by landing an assertion in DeviceContext() to check valid threadedness of the nsDeviceContext. I'm slightly nervous about poking around with exploratory asserts, as it points out possible security problems, but maybe it isn't a big deal.
> if either of those fields changes in between line 4347 and 4359 Given the code I'm pretty sure that can't happen. Also, the pres context is held strongly by the caller (CanvasRenderingContext2D::DrawWindow). So I believe it must be corrupt already at 4347. Reading CanvasRenderingContext2D::DrawWindow I see one bug: http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/content/canvas/src/CanvasRenderingContext2D.cpp#l3961 presContext->PresShell()->RenderDocument(...) This is wrong, the caller MUST guarantee liveness for the shell while calling RenderDocument, otherwise the shell might be deleted and 'mIsDestroying' is garbage here: http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/layout/base/nsPresShell.cpp#l4283 It seems somewhat unlikely that we would avoid crashing to reach line 4347 with a deleted 'this', then use a garbage mPresContext ptr. But I guess it could happen in theory; in that case it seems likely there would be some crashes also in PresShell::RenderDocument, and there is: https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&query_search=signature&query_type=contains&query=PresShell%3A%3ARenderDocument&reason=&build_id=&process_type=any&hang_type=any&do_query=1 (eg bug 747602) We should fix the RenderDocument() call site anyway, so let's start there and see if it helps.
Created attachment 675726 [details] [diff] [review] fix see last comment
Attachment #675726 - Flags: review?(roc)
Comment on attachment 675726 [details] [diff] [review] fix Review of attachment 675726 [details] [diff] [review]: ----------------------------------------------------------------- good catch(es)
Attachment #675726 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aebe2c45ef Leave open because this is a tentative fix. We should look at crash-stats for PresShell::RenderDocument and our own "orange crashes" like bug 747602 in a few weeks time and see if it seems to have fixed it.
Assignee: nobody → matspal
Whiteboard: [leave open]
speculatively adding keywords based on the fix
Keywords: csec-uaf, sec-critical
Created attachment 675890 [details] [diff] [review] for aurora/beta On aurora/beta there is also a RenderDocument() call in nsCanvasRenderingContext2DAzure.cpp which needs the same fix
Attachment #675890 - Flags: review?(roc)
Created attachment 675891 [details] [diff] [review] for esr10 esr10 is slightly different
Attachment #675891 - Flags: review?
Attachment #675890 - Flags: review?(roc) → review+
Attachment #675891 - Flags: review?(roc) → review+
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
No Bug 776505 crashes in the 1.5 days since this landed, which is promising.
Comment on attachment 675891 [details] [diff] [review] for esr10 [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: crash Testing completed (on m-c, etc.): on m-c since 2012-10-27 Risk to taking this patch (and alternatives if risky): very low risk String or UUID changes made by this patch: none
Attachment #675891 - Flags: approval-mozilla-esr10?
Comment on attachment 675890 [details] [diff] [review] for aurora/beta [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: crash Testing completed (on m-c, etc.): on m-c since 2012-10-27 Risk to taking this patch (and alternatives if risky): very low risk String or UUID changes made by this patch: none
tracking-firefox-esr10: ? → 17+
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Attachment #675891 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d5b6fef4452 https://hg.mozilla.org/releases/mozilla-beta/rev/49fe7370db22 https://hg.mozilla.org/releases/mozilla-esr10/rev/365e0576b2e7
status-firefox-esr10: affected → fixed
status-firefox16: affected → wontfix
status-firefox17: affected → fixed
status-firefox18: affected → fixed
Whiteboard: [leave open] → [leave open][adv-track-main17+][adv-track-esr17+]
The underlying orange has gone away, so it seems like this can be closed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+]
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.