805957 - Possible memory corruption of nsDeviceContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Mats Palmgren (inactive)]

> 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.

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

see last comment

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 675726 [details] [diff] [review]
fix

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

good catch(es)

Comment 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Andrew McCreight [:mccr8]]

speculatively adding keywords based on the fix

Comment 6

[Mats Palmgren (inactive)]

Attachment: for aurora/beta

On aurora/beta there is also a RenderDocument() call in
nsCanvasRenderingContext2DAzure.cpp which needs the same fix

Comment 7

[Mats Palmgren (inactive)]

Attachment: for esr10

esr10 is slightly different

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7aebe2c45ef

Comment 9

[Andrew McCreight [:mccr8]]

No Bug 776505 crashes in the 1.5 days since this landed, which is promising.

Comment 10

[Mats Palmgren (inactive)]

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

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Andrew McCreight [:mccr8]]

The underlying orange has gone away, so it seems like this can be closed.